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 2e0f058..c9ffc59 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 @@ -1053,6 +1053,12 @@ public static void multiply(Decimal128 left, Decimal128 right, } /** + * As of 2/11/2014 this has a known bug in multiplication. 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 multiply. + * * Performs multiplication, changing the scale of this object to the specified * value. * @@ -1061,7 +1067,7 @@ public static void multiply(Decimal128 left, Decimal128 right, * @param newScale * scale of the result. must be 0 or positive. */ - public void multiplyDestructive(Decimal128 right, short newScale) { + public void multiplyDestructiveNativeDecimal128(Decimal128 right, short newScale) { if (this.signum == 0 || right.signum == 0) { this.zeroClear(); this.scale = newScale; @@ -1102,6 +1108,31 @@ public void multiplyDestructive(Decimal128 right, short newScale) { } /** + * Performs multiplication, changing the scale of this object to the specified + * value. + * + * @param right + * right operand. this object is not modified. + * @param newScale + * scale of the result. must be 0 or positive. + */ + public void multiplyDestructive(Decimal128 right, short newScale) { + HiveDecimal rightHD = HiveDecimal.create(right.toBigDecimal()); + HiveDecimal thisHD = HiveDecimal.create(this.toBigDecimal()); + HiveDecimal result = thisHD.multiply(rightHD); + + /* If the result is null, throw an exception. This can be caught + * by calling code in the vectorized code path and made to yield + * a SQL NULL value. + */ + if (result == null) { + throw new ArithmeticException("null multiply result"); + } + this.update(result.bigDecimalValue().toPlainString(), newScale); + this.unscaledValue.throwIfExceedsTenToThirtyEight(); + } + + /** * Performs division and puts the result into the given object. Both operands * are first scaled up/down to the specified scale, then this method performs * the operation. This method is static and not destructive (except the result 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 6824cd7..426c03d 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 @@ -51,39 +51,19 @@ public void tearDown() throws Exception { } @Test - public void testCalculateTenThirtyEight() { + public void testCalculateTenThirtySeven() { - // find 10^38 + // find 10^37 Decimal128 ten = new Decimal128(10, (short) 0); Decimal128 val = new Decimal128(1, (short) 0); - for (int i = 0; i < 38; ++i) { + for (int i = 0; i < 37; ++i) { val.multiplyDestructive(ten, (short) 0); } // verify it String s = val.toFormalString(); - assertEquals("100000000000000000000000000000000000000", s); + assertEquals("10000000000000000000000000000000000000", 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 @@ -377,6 +357,13 @@ public void testKnownPriorErrors() { long a = 213474114411690L; long b = 5062120663L; verifyMultiplyDivideInverse(a, b); + + // Regression test for defect reported in HIVE-6399 + String a2 = "-605044214913338382"; // 18 digits + String b2 = "55269579109718297360"; // 20 digits + + // -33440539101030154945490585226577271520 is expected result + verifyHighPrecisionMultiplySingle(a2, b2); } // Test a set of random adds at high precision. @@ -415,7 +402,8 @@ private void verifyHighPrecisionAddSingle() { String res2 = bdR.toPlainString(); // Compare the results - assertEquals(res1, res2); + String message = "For operation " + a.toFormalString() + " + " + b.toFormalString(); + assertEquals(message, res2, res1); } // Test a set of random subtracts at high precision. @@ -454,7 +442,8 @@ private void verifyHighPrecisionSubtractSingle() { String res2 = bdR.toPlainString(); // Compare the results - assertEquals(res1, res2); + String message = "For operation " + a.toFormalString() + " - " + b.toFormalString(); + assertEquals(message, res2, res1); } // Test a set of random multiplications at high precision. @@ -490,7 +479,7 @@ private void verifyHighPrecisionMultiplySingle() { String res1 = r.toFormalString(); - // Now do the add with Java BigDecimal + // Now do the operation with Java BigDecimal BigDecimal bdA = new BigDecimal(sA); BigDecimal bdB = new BigDecimal(sB); BigDecimal bdR = bdA.multiply(bdB); @@ -498,9 +487,52 @@ private void verifyHighPrecisionMultiplySingle() { String res2 = bdR.toPlainString(); // Compare the results - assertEquals(res1, res2); + String message = "For operation " + a.toFormalString() + " * " + b.toFormalString(); + assertEquals(message, res2, res1); } + // Test a single, high-precision multiply of random inputs. + // Arguments must be integers with optional - sign, represented as strings. + // Arguments must have 1 to 37 digits and the number of total digits + // must be <= 38. + private void verifyHighPrecisionMultiplySingle(String argA, String argB) { + + Decimal128 a, b, r; + String sA, sB; + + // verify number of digits is <= 38 and each number has 1 or more digits + int aDigits = argA.length(); + aDigits -= argA.charAt(0) == '-' ? 1 : 0; + int bDigits = argB.length(); + bDigits -= argB.charAt(0) == '-' ? 1 : 0; + assertTrue(aDigits + bDigits <= 38 && aDigits > 0 && bDigits > 0); + + a = new Decimal128(); + sA = argA; + a.update(sA, (short) 0); + b = new Decimal128(); + sB = argB; + b.update(sB, (short) 0); + + r = new Decimal128(); + r.addDestructive(a, (short) 0); + r.multiplyDestructive(b, (short) 0); + + String res1 = r.toFormalString(); + + // Now do the operation with Java BigDecimal + BigDecimal bdA = new BigDecimal(sA); + BigDecimal bdB = new BigDecimal(sB); + BigDecimal bdR = bdA.multiply(bdB); + + String res2 = bdR.toPlainString(); + + // Compare the results + String message = "For operation " + a.toFormalString() + " * " + b.toFormalString(); + assertEquals(message, res2, res1); + } + + // Test a set of random divisions at high precision. @Test public void testHighPrecisionDecimal128Divide() { @@ -558,7 +590,8 @@ private void verifyHighPrecisionDivideSingle() { String res2 = bdR.toPlainString(); // Compare the results - assertEquals(res1, res2); + String message = "For operation " + a.toFormalString() + " / " + b.toFormalString(); + assertEquals(message, res2, res1); } /* Return a random number with length digits, as a string. Results may be