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 <dhe...@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 <tbobrovyt...@cloudera.com>
Authored: Wed Jan 17 20:32:11 2018 -0800
Committer: Impala Public Jenkins <impala-public-jenk...@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

Reply via email to