hive-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From eh...@apache.org
Subject svn commit: r1569111 - in /hive/trunk/common/src: java/org/apache/hadoop/hive/common/type/Decimal128.java test/org/apache/hadoop/hive/common/type/TestDecimal128.java
Date Mon, 17 Feb 2014 20:50:26 GMT
Author: ehans
Date: Mon Feb 17 20:50:25 2014
New Revision: 1569111

URL: http://svn.apache.org/r1569111
Log:
HIVE-6399: bug in high-precision Decimal128 multiply (Eric Hanson, reviewed by Jitendra Pandey)

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

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=1569111&r1=1569110&r2=1569111&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 Feb
17 20:50:25 2014
@@ -1053,6 +1053,12 @@ public final class Decimal128 extends Nu
   }
 
   /**
+   * 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 final class Decimal128 extends Nu
    * @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 final class Decimal128 extends Nu
   }
 
   /**
+   * 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

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=1569111&r1=1569110&r2=1569111&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
Feb 17 20:50:25 2014
@@ -51,39 +51,19 @@ public class TestDecimal128 {
   }
 
   @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 class TestDecimal128 {
     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 @@ public class TestDecimal128 {
     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 @@ public class TestDecimal128 {
     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 @@ public class TestDecimal128 {
 
     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 @@ public class TestDecimal128 {
     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 @@ public class TestDecimal128 {
     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



Mime
View raw message