diff --git a/common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java b/common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java index 3939511..15025cf 100644 --- a/common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java +++ b/common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java @@ -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 void multiplyDestructive(Decimal128 right, short newScale) { * 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. *
@@ -1128,7 +1133,7 @@ public static void divide(Decimal128 left, Decimal128 right, * @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 void divideDestructive(Decimal128 right, short newScale, } /** + * 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. */ diff --git a/common/src/test/org/apache/hadoop/hive/common/type/TestDecimal128.java b/common/src/test/org/apache/hadoop/hive/common/type/TestDecimal128.java index 3c05352..42d5d61 100644 --- a/common/src/test/org/apache/hadoop/hive/common/type/TestDecimal128.java +++ b/common/src/test/org/apache/hadoop/hive/common/type/TestDecimal128.java @@ -17,6 +17,8 @@ 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 void tearDown() throws Exception { @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 @@ private void assertNotEquals(double a, double b, double epsilon) { @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, (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); + } + - Decimal128.divide(three, four, quotient, remainder, (short) 0); - assertEquals("0", quotient.toFormalString()); - assertEquals("3", remainder.toFormalString()); + @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 void testPiNewton() { 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 void testPiArcsine() { 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 void testPiArcsine() { @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 void testDoubleValue() { @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 void testPrecisionOverflow() { 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) { + } } } diff --git a/ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/DecimalUtil.java b/ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/DecimalUtil.java index 1356ab5..48ec13a 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/DecimalUtil.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/DecimalUtil.java @@ -63,26 +63,10 @@ public static void multiplyChecked(int i, Decimal128 left, Decimal128 right, } // 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) { - 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) { + DecimalColumnVector outputColVector) { 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;