You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hive.apache.org by eh...@apache.org on 2014/01/27 18:30:15 UTC
svn commit: r1561760 - in /hive/trunk:
common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java
common/src/test/org/apache/hadoop/hive/common/type/TestDecimal128.java
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/DecimalUtil.java
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;