FAQ
Author: ehans
Date: Mon Jan 27 17:30:14 2014
New Revision: 1561760

URL: http://svn.apache.org/r1561760
Log:
HIVE-6243: error in high-precision division for Decimal128 (Eric Hanson)

Modified:
     hive/trunk/common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java
     hive/trunk/common/src/test/org/apache/hadoop/hive/common/type/TestDecimal128.java
     hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/DecimalUtil.java

Modified: hive/trunk/common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java
URL: http://svn.apache.org/viewvc/hive/trunk/common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java?rev=1561760&r1=1561759&r2=1561760&view=diff
==============================================================================
--- hive/trunk/common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java (original)
+++ hive/trunk/common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java Mon Jan 27 17:30:14 2014
@@ -16,6 +16,7 @@
  package org.apache.hadoop.hive.common.type;

  import java.math.BigDecimal;
+import java.math.MathContext;
  import java.nio.IntBuffer;

  /**
@@ -1097,23 +1098,27 @@ public final class Decimal128 extends Nu
     * right operand
     * @param quotient
     * result object to receive the calculation result
- * @param remainder
- * result object to receive the calculation result
     * @param scale
     * scale of the result. must be 0 or positive.
     */
    public static void divide(Decimal128 left, Decimal128 right,
- Decimal128 quotient, Decimal128 remainder, short scale) {
+ Decimal128 quotient, short scale) {
      if (quotient == left || quotient == right) {
        throw new IllegalArgumentException(
            "result object cannot be left or right operand");
      }

      quotient.update(left);
- quotient.divideDestructive(right, scale, remainder);
+ quotient.divideDestructive(right, scale);
    }

    /**
+ * As of 1/20/2014 this has a known bug in division. See
+ * TestDecimal128.testKnownPriorErrors(). Keeping this source
+ * code available since eventually it is better to fix this.
+ * The fix employed now is to replace this code with code that
+ * uses Java BigDecimal divide.
+ *
     * Performs division, changing the scale of this object to the specified
     * value.
     * <p>
@@ -1128,7 +1133,7 @@ public final class Decimal128 extends Nu
     * @param remainder
     * object to receive remainder
     */
- public void divideDestructive(Decimal128 right, short newScale,
+ public void divideDestructiveNativeDecimal128(Decimal128 right, short newScale,
        Decimal128 remainder) {
      if (right.signum == 0) {
        SqlMathUtil.throwZeroDivisionException();
@@ -1174,6 +1179,23 @@ public final class Decimal128 extends Nu
    }

    /**
+ * Divide the target object by right, and scale the result to newScale.
+ *
+ * This uses HiveDecimal to get a correct answer with the same rounding
+ * behavior as HiveDecimal, but it is expensive.
+ *
+ * In the future, a native implementation could be faster.
+ */
+ public void divideDestructive(Decimal128 right, short newScale) {
+ HiveDecimal rightHD = HiveDecimal.create(right.toBigDecimal());
+ HiveDecimal thisHD = HiveDecimal.create(this.toBigDecimal());
+ HiveDecimal result = thisHD.divide(rightHD);
+ this.update(result.bigDecimalValue().toPlainString(), newScale);
+ this.unscaledValue.throwIfExceedsTenToThirtyEight();
+ }
+
+
+ /**
     * Makes this {@code Decimal128} a positive number. Unlike
     * java.math.BigDecimal, this method is destructive.
     */

Modified: hive/trunk/common/src/test/org/apache/hadoop/hive/common/type/TestDecimal128.java
URL: http://svn.apache.org/viewvc/hive/trunk/common/src/test/org/apache/hadoop/hive/common/type/TestDecimal128.java?rev=1561760&r1=1561759&r2=1561760&view=diff
==============================================================================
--- hive/trunk/common/src/test/org/apache/hadoop/hive/common/type/TestDecimal128.java (original)
+++ hive/trunk/common/src/test/org/apache/hadoop/hive/common/type/TestDecimal128.java Mon Jan 27 17:30:14 2014
@@ -17,6 +17,8 @@ package org.apache.hadoop.hive.common.ty

  import static org.junit.Assert.*;

+import java.util.Random;
+
  import org.junit.After;
  import org.junit.Before;
  import org.junit.Test;
@@ -47,11 +49,38 @@ public class TestDecimal128 {

    @Test
    public void testCalculateTenThirtyEight() {
+
+ // find 10^38
      Decimal128 ten = new Decimal128(10, (short) 0);
      Decimal128 val = new Decimal128(1, (short) 0);
      for (int i = 0; i < 38; ++i) {
        val.multiplyDestructive(ten, (short) 0);
      }
+
+ // verify it
+ String s = val.toFormalString();
+ assertEquals("100000000000000000000000000000000000000", s);
+ boolean overflow = false;
+
+ // show that it is is an overflow for precision 38
+ try {
+ val.checkPrecisionOverflow(38);
+ } catch (Exception e) {
+ overflow = true;
+ }
+ assertTrue(overflow);
+
+ // subtract one
+ val.subtractDestructive(one, (short) 0);
+ overflow = false;
+
+ // show that it does not overflow for precision 38
+ try {
+ val.checkPrecisionOverflow(38);
+ } catch (Exception e) {
+ overflow = true;
+ }
+ assertFalse(overflow);
    }

    @Test
@@ -247,28 +276,104 @@ public class TestDecimal128 {
    @Test
    public void testDivide() {
      Decimal128 quotient = new Decimal128();
- Decimal128 remainder = new Decimal128();
- Decimal128.divide(two, one, quotient, remainder, (short) 2);
+ Decimal128.divide(two, one, quotient, (short) 2);
      assertEquals(0, quotient.compareTo(two));
- assertTrue(remainder.isZero());

- Decimal128.divide(two, two, quotient, remainder, (short) 2);
+ Decimal128.divide(two, two, quotient, (short) 2);
      assertEquals(0, quotient.compareTo(one));
- assertTrue(remainder.isZero());

      Decimal128 three = new Decimal128(3);
      Decimal128 four = new Decimal128(4);
- Decimal128.divide(three, four, quotient, remainder, (short) 2);
+ Decimal128.divide(three, four, quotient, (short) 2);
      assertEquals("0.75", quotient.toFormalString());
- assertEquals("0", remainder.toFormalString());

- Decimal128.divide(three, four, quotient, remainder, (short) 1);
- assertEquals("0.7", quotient.toFormalString());
- assertEquals("0.2", remainder.toFormalString());
-
- Decimal128.divide(three, four, quotient, remainder, (short) 0);
- assertEquals("0", quotient.toFormalString());
- assertEquals("3", remainder.toFormalString());
+ Decimal128.divide(three, four, quotient, (short) 1);
+ assertEquals("0.8", quotient.toFormalString());
+
+ Decimal128.divide(three, four, quotient, (short) 0);
+ assertEquals("1", quotient.toFormalString());
+
+ Decimal128 two = new Decimal128(2);
+ Decimal128.divide(two, three, quotient, (short) 4);
+ assertEquals("0.6667", quotient.toFormalString());
+ }
+
+ @Test
+ public void testRandomMultiplyDivideInverse() {
+ final int N = 100000;
+ final long MASK56 = 0x00FFFFFFFFFFFFL; // 56 bit mask to generate positive 56 bit longs
+ // from random signed longs
+ int seed = 897089790;
+ Random rand = new Random(seed);
+ long l1, l2;
+ for (int i = 1; i <= N; i++) {
+ l1 = rand.nextLong() & MASK56;
+ l2 = rand.nextLong() & MASK56;
+ verifyMultiplyDivideInverse(l1, l2);
+ }
+ }
+
+ /**
+ * Verify that a * b / b == a
+ * for decimal division for scale 0 with integer inputs.
+ *
+ * Not valid if abs(a * b) >= 10**38.
+ */
+ private void verifyMultiplyDivideInverse(long a, long b) {
+ final short scale = 0;
+
+ // ignore zero-divide cases
+ if (b == 0) {
+ return;
+ }
+ Decimal128 decA = new Decimal128(a, scale);
+ Decimal128 decB = new Decimal128(b, scale);
+ decA.multiplyDestructive(decB, scale);
+ decA.checkPrecisionOverflow(38); // caller must make sure product of inputs is not too big
+ decA.divideDestructive(decB, scale);
+ assertEquals("Error for a = " + Long.toString(a) + ", b = " + Long.toString(b),
+ new Decimal128(a, scale), decA);
+ }
+
+
+ @Test
+ public void testRandomAddSubtractInverse() {
+ final int N = 1000000;
+ int seed = 1427480960;
+ Random rand = new Random(seed);
+ long l1, l2;
+ for (int i = 1; i <= N; i++) {
+ l1 = rand.nextLong();
+ l2 = rand.nextLong();
+ verifyAddSubtractInverse(l1, l2);
+ }
+ }
+
+ /**
+ * Verify that (a + b) - b == a
+ * for decimal add and subtract for scale 0 with long integer inputs.
+ */
+ private void verifyAddSubtractInverse(long a, long b) {
+ final short scale = 0;
+ Decimal128 decA = new Decimal128(a, scale);
+ Decimal128 decB = new Decimal128(b, scale);
+ decA.addDestructive(decB, scale);
+
+ decA.subtractDestructive(decB, scale);
+ assertEquals("Error for a = " + Long.toString(a) + ", b = " + Long.toString(b),
+ new Decimal128(a, scale), decA);
+ }
+
+ /**
+ * During earlier code testing, if we found errors, test them here as regression tests.
+ */
+ @Test
+ public void testKnownPriorErrors() {
+
+ // Regression test for defect reported in HIVE-6243
+ long a = 213474114411690L;
+ long b = 5062120663L;
+ verifyMultiplyDivideInverse(a, b);
    }

    @Test
@@ -281,13 +386,12 @@ public class TestDecimal128 {
      Decimal128 current = new Decimal128(1, SCALE);
      Decimal128 multiplier = new Decimal128();
      Decimal128 dividor = new Decimal128();
- Decimal128 remainder = new Decimal128();
      Decimal128 one = new Decimal128(1);
      for (int i = LOOPS; i > 0; --i) {
        multiplier.update(i, SCALE);
        current.multiplyDestructive(multiplier, SCALE);
        dividor.update(1 + 2 * i, SCALE);
- current.divideDestructive(dividor, SCALE, remainder);
+ current.divideDestructive(dividor, SCALE);
        current.addDestructive(one, SCALE);
      }
      current.multiplyDestructive(new Decimal128(2), SCALE);
@@ -307,17 +411,16 @@ public class TestDecimal128 {
      Decimal128 total = new Decimal128(0);
      Decimal128 multiplier = new Decimal128();
      Decimal128 dividor = new Decimal128();
- Decimal128 remainder = new Decimal128();
      Decimal128 current = new Decimal128();
      for (int i = 0; i < LOOPS; ++i) {
        current.update(3, SCALE);
        dividor.update(2 * i + 1, SCALE);
- current.divideDestructive(dividor, SCALE, remainder);
+ current.divideDestructive(dividor, SCALE);
        for (int j = 1; j <= i; ++j) {
          multiplier.update(i + j, SCALE);
          dividor.update(16 * j, SCALE);
          current.multiplyDestructive(multiplier, SCALE);
- current.divideDestructive(dividor, SCALE, remainder);
+ current.divideDestructive(dividor, SCALE);
        }

        total.addDestructive(current, SCALE);
@@ -329,16 +432,15 @@ public class TestDecimal128 {
    @Test
    public void testDoubleValue() {
      Decimal128 quotient = new Decimal128();
- Decimal128 remainder = new Decimal128();

      Decimal128 three = new Decimal128(3);
      Decimal128 four = new Decimal128(9);
- Decimal128.divide(three, four, quotient, remainder, (short) 38);
+ Decimal128.divide(three, four, quotient, (short) 38);
      assertEquals(0.33333333333333333333333333d, quotient.doubleValue(),
          0.0000000000000000000000001d);

      Decimal128 minusThree = new Decimal128(-3);
- Decimal128.divide(minusThree, four, quotient, remainder, (short) 38);
+ Decimal128.divide(minusThree, four, quotient, (short) 38);
      assertEquals(-0.33333333333333333333333333d, quotient.doubleValue(),
          0.0000000000000000000000001d);
    }
@@ -346,15 +448,14 @@ public class TestDecimal128 {
    @Test
    public void testFloatValue() {
      Decimal128 quotient = new Decimal128();
- Decimal128 remainder = new Decimal128();

      Decimal128 three = new Decimal128(3);
      Decimal128 four = new Decimal128(9);
- Decimal128.divide(three, four, quotient, remainder, (short) 38);
+ Decimal128.divide(three, four, quotient, (short) 38);
      assertEquals(0.3333333333333333f, quotient.floatValue(), 0.00000000001f);

      Decimal128 minusThree = new Decimal128(-3);
- Decimal128.divide(minusThree, four, quotient, remainder, (short) 38);
+ Decimal128.divide(minusThree, four, quotient, (short) 38);
      assertEquals(-0.333333333333333f, quotient.floatValue(), 0.00000000001f);
    }

@@ -410,5 +511,29 @@ public class TestDecimal128 {
        fail();
      } catch (ArithmeticException ex) {
      }
+
+ // Try the extremes of precision and scale.
+
+ // digit measuring stick:
+ // 12345678901234567890123456789012345678
+ new Decimal128("0.99999999999999999999999999999999999999", (short) 38)
+ .checkPrecisionOverflow(38);
+
+ try {
+ new Decimal128("0.99999999999999999999999999999999999999", (short) 38)
+ .checkPrecisionOverflow(37);
+ fail();
+ } catch (ArithmeticException ex) {
+ }
+
+ new Decimal128("99999999999999999999999999999999999999", (short) 0)
+ .checkPrecisionOverflow(38);
+
+ try {
+ new Decimal128("99999999999999999999999999999999999999", (short) 0)
+ .checkPrecisionOverflow(37);
+ fail();
+ } catch (ArithmeticException ex) {
+ }
    }
  }

Modified: hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/DecimalUtil.java
URL: http://svn.apache.org/viewvc/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/DecimalUtil.java?rev=1561760&r1=1561759&r2=1561760&view=diff
==============================================================================
--- hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/DecimalUtil.java (original)
+++ hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/DecimalUtil.java Mon Jan 27 17:30:14 2014
@@ -63,26 +63,10 @@ public class DecimalUtil {
    }

    // Division with overflow/zero-divide check. Error produces NULL output.
- // Remainder argument is necessary to match up with the Decimal128.divide() interface.
- // It will be discarded so just pass in a dummy argument.
    public static void divideChecked(int i, Decimal128 left, Decimal128 right,
- DecimalColumnVector outputColVector, Decimal128 remainder) {
+ DecimalColumnVector outputColVector) {
      try {
- Decimal128.divide(left, right, outputColVector.vector[i], remainder, outputColVector.scale);
- outputColVector.vector[i].checkPrecisionOverflow(outputColVector.precision);
- } catch (ArithmeticException e) { // catch on error
- outputColVector.noNulls = false;
- outputColVector.isNull[i] = true;
- }
- }
-
- // Modulo operator with overflow/zero-divide check.
- // Quotient argument is necessary to match up with Decimal128.divide() interface.
- // It will be discarded so just pass in a dummy argument.
- public static void moduloChecked(int i, Decimal128 left, Decimal128 right,
- DecimalColumnVector outputColVector, Decimal128 quotient) {
- try {
- Decimal128.divide(left, right, quotient, outputColVector.vector[i], outputColVector.scale);
+ Decimal128.divide(left, right, outputColVector.vector[i], outputColVector.scale);
        outputColVector.vector[i].checkPrecisionOverflow(outputColVector.precision);
      } catch (ArithmeticException e) { // catch on error
        outputColVector.noNulls = false;

Search Discussions

Related Discussions

Discussion Navigation
viewthread | post
posts ‹ prev | 1 of 1 | next ›
Discussion Overview
groupcommits @
categorieshive, hadoop
postedJan 27, '14 at 5:30p
activeJan 27, '14 at 5:30p
posts1
users1
websitehive.apache.org

1 user in discussion

Ehans: 1 post

People

Translate

site design / logo © 2021 Grokbase