[1/6] impala git commit: IMPALA-6405: Error when string to decimal cast overflows

2018-03-13 Thread tarmstrong 
Repository: impala
Updated Branches:
  refs/heads/2.x 0ba181e5b -> 6d3fdee3f


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.

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 
Tested-by: Impala Public Jenkins
Reviewed-on: http://gerrit.cloudera.org:8080/9530
Reviewed-by: Taras Bobrovytsky 


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

Branch: refs/heads/2.x
Commit: 95d509faeddaf2552d90530b9f111ed3adfebfda
Parents: 0ba181e
Author: Taras Bobrovytsky 
Authored: Wed Jan 17 20:32:11 2018 -0800
Committer: Lars Volker 
Committed: Mon Mar 12 23:28:52 2018 +

--
 be/src/exprs/decimal-operators-ir.cc| 26 +++
 .../rewrite/RemoveRedundantStringCast.java  |  3 +-
 .../queries/QueryTest/decimal-exprs.test| 33 
 3 files changed, 55 insertions(+), 7 deletions(-)
--


http://git-wip-us.apache.org/repos/asf/impala/blob/95d509fa/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/95d509fa/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/95d509fa/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 be75c23..41441e5 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(123 as decimal(38, 0)) as timestamp);
  TYPES
 TIMESTAMP, TIMESTAMP, TIMESTAMP, TIMESTAMP, TIMESTAMP,

impala git commit: IMPALA-6405: Error when string to decimal cast overflows

2018-03-06 Thread tarmstrong 
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 
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 
Authored: Wed Jan 17 20:32:11 2018 -0800
Committer: Impala Public Jenkins 
Committed: Tue Mar 6 03:29:47 2018 +

--
 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(9.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() &&