impala-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From k...@apache.org
Subject [2/2] impala git commit: IMPALA-6292: Fix incorrect DCHECK in decimal subtraction
Date Fri, 08 Dec 2017 08:06:52 GMT
IMPALA-6292: Fix incorrect DCHECK in decimal subtraction

When subtracting two decimals, and one of them is large, we incorrectly
hit a DCHECK if the intermediate result was equal to 10^39-1, which is
MAX_UNSCALED_DECIMAL16. We fix the problem by changing the condition in
the DCHECK from "less than" to "less than or equal to".

Testing:
- Added expr tests that reproduce the issue.

Change-Id: I42d42ad85efe32b7a0db0d2353385939fee09934
Reviewed-on: http://gerrit.cloudera.org:8080/8796
Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com>
Tested-by: Impala Public Jenkins


Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/542dc227
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/542dc227
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/542dc227

Branch: refs/heads/master
Commit: 542dc227c613e2fbe2b9a4f98817f395b2d1238c
Parents: d60eb19
Author: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Authored: Thu Dec 7 16:51:35 2017 -0800
Committer: Impala Public Jenkins <impala-public-jenkins@gerrit.cloudera.org>
Committed: Fri Dec 8 05:02:08 2017 +0000

----------------------------------------------------------------------
 be/src/exprs/expr-test.cc             | 23 ++++++++++++++++-------
 be/src/runtime/decimal-value.inline.h |  6 +++---
 2 files changed, 19 insertions(+), 10 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/542dc227/be/src/exprs/expr-test.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc
index 4b7bcd0..5670bcf 100644
--- a/be/src/exprs/expr-test.cc
+++ b/be/src/exprs/expr-test.cc
@@ -1611,7 +1611,7 @@ DecimalTestCase decimal_cases[] = {
     {{ false, false, StringToInt128("-85070591730234615865843651857942052863"), 38, 0 }}},
   { "cast(42535295865117307932921825928971026432 as decimal(38,0)) + "
     "cast(-42535295865117307932921825928971026431 as decimal(38,0))",
-    {{ false, false, StringToInt128("1"), 38, 0 }}},
+    {{ false, false, 1, 38, 0 }}},
   { "cast(99999999999999999999999999999999.999999 as decimal(38,6)) + "
     "cast(8899999999999999999999999999999.999999 as decimal(38,6))",
     {{ false, true, 0, 38, 6 },
@@ -1647,10 +1647,10 @@ DecimalTestCase decimal_cases[] = {
     {{ false, false, StringToInt128("22222222222222222222222222222223333332"), 38, 6 }}},
   { "cast(-11111111111111111111111111111111.777777 as decimal(38,6)) + "
     "cast(11111111111111111111111111111111.555555 as decimal(38,6))",
-    {{ false, false, StringToInt128("-222222"), 38, 6 }}},
+    {{ false, false, -222222, 38, 6 }}},
   { "cast(11111111111111111111111111111111.777777 as decimal(38,6)) + "
     "cast(-11111111111111111111111111111111.555555 as decimal(38,6))",
-    {{ false, false, StringToInt128("222222"), 38, 6 }}},
+    {{ false, false, 222222, 38, 6 }}},
   { "cast(-11111111111111111111111111111111.777777 as decimal(38,6)) + "
     "cast(-11111111111111111111111111111111.555555 as decimal(38,6))",
     {{ false, false, StringToInt128("-22222222222222222222222222222223333332"), 38, 6 }}},
@@ -1721,11 +1721,20 @@ DecimalTestCase decimal_cases[] = {
      { false, false, StringToInt128("20000000000000000000000000000000000000"), 38, 37 }}},
   { "cast(-0.99999999999999999999999999999999999999 as decimal(38,38)) + "
     "cast(0.99999999999999999999999999999999999999 as decimal(38,38))",
-    {{ false, false, StringToInt128("0"), 38, 38 },
-     { false, false, StringToInt128("0"), 38, 37 }}},
+    {{ false, false, 0, 38, 38 },
+     { false, false, 0, 38, 37 }}},
   { "cast(0 as decimal(38,38)) + cast(0 as decimal(38,38))",
-    {{ false, false, StringToInt128("0"), 38, 38 },
-     { false, false, StringToInt128("0"), 38, 37 }}},
+    {{ false, false, 0, 38, 38 },
+     { false, false, 0, 38, 37 }}},
+  // IMPALA-6292
+  { "cast(1 as decimal(13,12)) - "
+    "cast(0.99999999999999999999999999999999999999 as decimal(38,38))",
+     {{ false, false, 1, 38, 38 },
+      { false, false, 0, 38, 36 }}},
+  { "cast(0.1 as decimal(13,12)) - "
+    "cast(99999999999999999999999999999999999999 as decimal(38,0))",
+     {{ false, true, 0, 38, 12 },
+      { true, false, 0, 38, 6 }}},
   // Test multiply operator
   { "cast(1.23 as decimal(30,2)) * cast(1 as decimal(10,3))",
     {{ false, false, 123000, 38, 5 }}},

http://git-wip-us.apache.org/repos/asf/impala/blob/542dc227/be/src/runtime/decimal-value.inline.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/decimal-value.inline.h b/be/src/runtime/decimal-value.inline.h
index 2e17d96..8f76335 100644
--- a/be/src/runtime/decimal-value.inline.h
+++ b/be/src/runtime/decimal-value.inline.h
@@ -267,12 +267,12 @@ inline int128_t SubtractLarge(int128_t x, int x_scale, int128_t y, int
y_scale,
   int result_scale_decrease = max_scale - result_scale;
   DCHECK_GE(result_scale_decrease, 0);
 
-  right = x_right + y_right;
   left = x_left + y_left;
+  right = x_right + y_right;
   // Overflow is not possible because one number is positive and the other one is
   // negative.
-  DCHECK(abs(right) < DecimalUtil::MAX_UNSCALED_DECIMAL16);
-  DCHECK(abs(left) < DecimalUtil::MAX_UNSCALED_DECIMAL16);
+  DCHECK(abs(left) <= DecimalUtil::MAX_UNSCALED_DECIMAL16);
+  DCHECK(abs(right) <= DecimalUtil::MAX_UNSCALED_DECIMAL16);
   // If the whole and fractional parts have different signs, then we need to make the
   // fractional part have the same sign as the whole part. If either left or right is
   // zero, then nothing needs to be done.


Mime
View raw message