You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2018/03/06 16:11:45 UTC
impala git commit: IMPALA-6405: Error when string to decimal cast
overflows
Repository: impala
Updated Branches:
refs/heads/master 5f2f445e7 -> b0027575c
IMPALA-6405: Error when string to decimal cast overflows
Before this patch, when there was an error when converting a string to
a decimal, a NULL was returned. In this patch, we change this behavior
so that an error is returned if decimal_v2 is enabled. We also add a
warning if there is an underflow.
The reasoning is that we want stricter behavior in decimal_v2.
Testing:
- Added some EE tests.
- Ran an exhaustive build, which passed.
Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Reviewed-on: http://gerrit.cloudera.org:8080/9339
Reviewed-by: Dan Hecht <dh...@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/b0027575
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/b0027575
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/b0027575
Branch: refs/heads/master
Commit: b0027575cbce7002ff867750e955ff3ecd3b1bcd
Parents: 5f2f445
Author: Taras Bobrovytsky <tb...@cloudera.com>
Authored: Wed Jan 17 20:32:11 2018 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Tue Mar 6 03:29:47 2018 +0000
----------------------------------------------------------------------
be/src/exprs/decimal-operators-ir.cc | 26 +++++++++++----
be/src/exprs/expr-test.cc | 3 +-
.../rewrite/RemoveRedundantStringCast.java | 3 +-
.../queries/QueryTest/decimal-exprs.test | 33 ++++++++++++++++++++
tests/query_test/test_decimal_casting.py | 7 +----
5 files changed, 57 insertions(+), 15 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/b0027575/be/src/exprs/decimal-operators-ir.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/decimal-operators-ir.cc b/be/src/exprs/decimal-operators-ir.cc
index fd0c404..0bde566 100644
--- a/be/src/exprs/decimal-operators-ir.cc
+++ b/be/src/exprs/decimal-operators-ir.cc
@@ -561,13 +561,27 @@ IR_ALWAYS_INLINE DecimalVal DecimalOperators::CastToDecimalVal(
DCHECK(false);
return DecimalVal::null();
}
- // Like all the cast functions, we return the truncated value on underflow and NULL
- // on overflow.
- // TODO: log warning on underflow.
- if (result == StringParser::PARSE_SUCCESS || result == StringParser::PARSE_UNDERFLOW) {
- return dv;
+
+ if (UNLIKELY(result == StringParser::PARSE_FAILURE)) {
+ if (is_decimal_v2) {
+ ctx->SetError("String to Decimal parse failed");
+ } else {
+ ctx->AddWarning("String to Decimal parse failed");
+ }
+ return DecimalVal::null();
+ }
+
+ if (UNLIKELY(result == StringParser::PARSE_OVERFLOW)) {
+ if (is_decimal_v2) {
+ ctx->SetError("String to Decimal cast overflowed");
+ } else {
+ ctx->AddWarning("String to Decimal cast overflowed");
+ }
+ return DecimalVal::null();
}
- return DecimalVal::null();
+
+ DCHECK(result == StringParser::PARSE_SUCCESS || StringParser::PARSE_UNDERFLOW);
+ return dv;
}
StringVal DecimalOperators::CastToStringVal(
http://git-wip-us.apache.org/repos/asf/impala/blob/b0027575/be/src/exprs/expr-test.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc
index 71f91c8..0b9a2e1 100644
--- a/be/src/exprs/expr-test.cc
+++ b/be/src/exprs/expr-test.cc
@@ -7973,8 +7973,7 @@ TEST_F(ExprTest, DecimalOverflowCastsDecimalV2) {
TestError("cast(99999999999999999999999999999.9 as decimal(29, 1))");
// Tests converting a non-trivial empty string to a decimal (IMPALA-1566).
- TestIsNull("cast(regexp_replace('','a','b') as decimal(15,2))",
- ColumnType::CreateDecimalType(15,2));
+ TestError("cast(regexp_replace('','a','b') as decimal(15,2))");
executor_->PopExecOption();
}
http://git-wip-us.apache.org/repos/asf/impala/blob/b0027575/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java b/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
index 40ab0fa..d305f27 100644
--- a/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
+++ b/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
@@ -82,7 +82,8 @@ public class RemoveRedundantStringCast implements ExprRewriteRule {
analyzer.getQueryCtx());
// Need to trim() while comparing char(n) types as conversion might add trailing
// spaces to the 'resultOfReverseCast'.
- if (!resultOfReverseCast.isNullLiteral() &&
+ if (resultOfReverseCast != null &&
+ !resultOfReverseCast.isNullLiteral() &&
resultOfReverseCast.getStringValue().trim()
.equals(literalExpr.getStringValue().trim())) {
return new BinaryPredicate(op, castExprChild,
http://git-wip-us.apache.org/repos/asf/impala/blob/b0027575/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test b/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
index dafe9ad..7c98819 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
@@ -454,3 +454,36 @@ cast(cast(12333333333 as decimal(38, 0)) as timestamp);
---- TYPES
TIMESTAMP, TIMESTAMP, TIMESTAMP, TIMESTAMP, TIMESTAMP, TIMESTAMP, TIMESTAMP, TIMESTAMP, TIMESTAMP, TIMESTAMP, TIMESTAMP, TIMESTAMP
====
+---- QUERY
+# IMPALA-6405: String to Decimal conversion errors
+set decimal_v2=false;
+select cast("abc" as decimal(5, 2));
+---- RESULTS
+NULL
+---- TYPES
+DECIMAL
+---- ERRORS
+UDF WARNING: String to Decimal parse failed
+====
+---- QUERY
+set decimal_v2=true;
+select cast("abc" as decimal(5, 2));
+---- CATCH
+UDF ERROR: String to Decimal parse failed
+====
+---- QUERY
+set decimal_v2=false;
+select cast("1234.5" as decimal(5, 2));
+---- RESULTS
+NULL
+---- TYPES
+DECIMAL
+---- ERRORS
+UDF WARNING: String to Decimal cast overflowed
+====
+---- QUERY
+set decimal_v2=true;
+select cast("1234.5" as decimal(5, 2));
+---- CATCH
+UDF ERROR: String to Decimal cast overflowed
+====
http://git-wip-us.apache.org/repos/asf/impala/blob/b0027575/tests/query_test/test_decimal_casting.py
----------------------------------------------------------------------
diff --git a/tests/query_test/test_decimal_casting.py b/tests/query_test/test_decimal_casting.py
index 3becc3f..752e3ad 100644
--- a/tests/query_test/test_decimal_casting.py
+++ b/tests/query_test/test_decimal_casting.py
@@ -137,12 +137,7 @@ class TestDecimalCasting(ImpalaTestSuite):
val = self._gen_decimal_val(from_precision, scale)
cast = self._normalize_cast_expr(val, from_precision,\
vector.get_value('cast_from')).format(val, precision, scale)
- if vector.get_value('cast_from') == "string":
- # TODO: This should be an error in both cases (IMPALA-6405).
- res = self.execute_scalar(cast)
- self._assert_decimal_result(cast, res, 'NULL')
- else:
- res = self.execute_query_expect_failure(self.client, cast)
+ res = self.execute_query_expect_failure(self.client, cast)
def test_underflow(self, vector):
"""Test to verify that we truncate when the scale of the number being cast is higher