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 <[email protected]> Reviewed-by: Jim Apple <[email protected]> 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 <[email protected]> Authored: Wed Jul 26 18:06:21 2017 -0700 Committer: Impala Public Jenkins <[email protected]> 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; }
