You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by mj...@apache.org on 2017/07/31 16:12:38 UTC
[3/6] incubator-impala git commit: IMPALA-5722: Fix string to decimal
cast
IMPALA-5722: Fix string to decimal cast
When converting a string to a decimal, we didn't handle the case where
the exponent is a large negative number. The function for computing the
divisor returns -1 when the exponent is too large. The problem is fixed
by making sure that the divisor is positive.
Testing:
-Added decimal tests.
Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Reviewed-on: http://gerrit.cloudera.org:8080/7517
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/86231459
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/86231459
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/86231459
Branch: refs/heads/master
Commit: 862314599614fd20f6e64ccb8600cd53f0f879d5
Parents: 2ee1606
Author: Taras Bobrovytsky <tb...@cloudera.com>
Authored: Wed Jul 26 18:06:21 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Sat Jul 29 09:43:47 2017 +0000
----------------------------------------------------------------------
be/src/runtime/decimal-test.cc | 9 ++++++++-
be/src/util/decimal-util.h | 5 +++++
be/src/util/string-parser.h | 23 ++++++++++++++++++-----
3 files changed, 31 insertions(+), 6 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/86231459/be/src/runtime/decimal-test.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/decimal-test.cc b/be/src/runtime/decimal-test.cc
index 190ee39..d81a16e 100644
--- a/be/src/runtime/decimal-test.cc
+++ b/be/src/runtime/decimal-test.cc
@@ -294,7 +294,14 @@ TEST(StringToDecimal, Basic) {
StringToAllDecimals("1.1.0e0", 10, 0, 100, StringParser::PARSE_FAILURE);
StringToAllDecimals("1e1.0", 10, 0, 100, StringParser::PARSE_FAILURE);
StringToAllDecimals("1..1e1", 10, 0, 100, StringParser::PARSE_FAILURE);
- StringToAllDecimals("1e9999999999999999999", 10, 0, 100, StringParser::PARSE_OVERFLOW);
+ StringToAllDecimals("1e9999999999999999999", 10, 0, 0, StringParser::PARSE_OVERFLOW);
+ StringToAllDecimals("1e-38", 10, 2, 0, StringParser::PARSE_UNDERFLOW);
+ StringToAllDecimals("1e-38", 38, 38, 1, StringParser::PARSE_SUCCESS);
+ StringToAllDecimals("-1e-38", 38, 38, -1, StringParser::PARSE_SUCCESS);
+ StringToAllDecimals("1e-39", 38, 38, 0, StringParser::PARSE_UNDERFLOW);
+ StringToAllDecimals("-1e-39", 38, 38, 0, StringParser::PARSE_UNDERFLOW);
+ StringToAllDecimals("1e-9999999999999999999", 10, 0, 0, StringParser::PARSE_UNDERFLOW);
+ StringToAllDecimals("-1e-9999999999999999999", 10, 0, 0, StringParser::PARSE_UNDERFLOW);
StringToAllDecimals(" 1e0 ", 10, 2, 100, StringParser::PARSE_SUCCESS);
StringToAllDecimals("-1e0", 10, 2, -100, StringParser::PARSE_SUCCESS);
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/86231459/be/src/util/decimal-util.h
----------------------------------------------------------------------
diff --git a/be/src/util/decimal-util.h b/be/src/util/decimal-util.h
index 9048793..288caf3 100644
--- a/be/src/util/decimal-util.h
+++ b/be/src/util/decimal-util.h
@@ -59,6 +59,11 @@ class DecimalUtil {
DCHECK_GE(scale, 0);
T result = 1;
for (int i = 0; i < scale; ++i) {
+ // Verify that the result of multiplication does not overflow.
+ // TODO: This is not an ideal way to check for overflow because if T is signed, the
+ // behavior is undefined in case of overflow. Replace this with a better overflow
+ // check.
+ DCHECK_GE(result * 10, result);
result *= 10;
}
return result;
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/86231459/be/src/util/string-parser.h
----------------------------------------------------------------------
diff --git a/be/src/util/string-parser.h b/be/src/util/string-parser.h
index 62e7dcc..cc9c517 100644
--- a/be/src/util/string-parser.h
+++ b/be/src/util/string-parser.h
@@ -142,7 +142,7 @@ class StringParser {
--len;
}
- // Ignore leading zeros even after a dot. This allows for differetiating between
+ // Ignore leading zeros even after a dot. This allows for differentiating between
// cases like 0.01e2, which would fit in a DECIMAL(1, 0), and 0.10e2, which would
// overflow.
int scale = 0;
@@ -182,7 +182,12 @@ class StringParser {
} else if ((c == 'e' || c == 'E') && LIKELY(!found_exponent)) {
found_exponent = true;
exponent = StringToIntInternal<int8_t>(s + i + 1, len - i - 1, result);
- if (UNLIKELY(*result != StringParser::PARSE_SUCCESS)) return DecimalValue<T>(0);
+ if (UNLIKELY(*result != StringParser::PARSE_SUCCESS)) {
+ if (*result == StringParser::PARSE_OVERFLOW && exponent < 0) {
+ *result = StringParser::PARSE_UNDERFLOW;
+ }
+ return DecimalValue<T>(0);
+ }
break;
} else {
*result = StringParser::PARSE_FAILURE;
@@ -216,9 +221,17 @@ class StringParser {
} else if (UNLIKELY(scale > type_scale)) {
*result = StringParser::PARSE_UNDERFLOW;
int shift = scale - type_scale;
- if (truncated_digit_count > 0) shift -= truncated_digit_count;
- if (shift > 0) value /= DecimalUtil::GetScaleMultiplier<T>(shift);
- DCHECK(value >= 0);
+ if (UNLIKELY(truncated_digit_count > 0)) shift -= truncated_digit_count;
+ if (shift > 0) {
+ T divisor = DecimalUtil::GetScaleMultiplier<T>(shift);
+ if (LIKELY(divisor >= 0)) {
+ value /= divisor;
+ } else {
+ DCHECK(divisor == -1); // DCHECK_EQ doesn't work with int128_t.
+ value = 0;
+ }
+ }
+ DCHECK(value >= 0); // DCHECK_GE doesn't work with int128_t.
} else if (UNLIKELY(!found_value && !found_dot)) {
*result = StringParser::PARSE_FAILURE;
}