You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org> on 2018/02/15 22:08:52 UTC
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9339
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
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 MULL was returned. In this patch, we change this behavior
so that an return in 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
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M tests/query_test/test_decimal_casting.py
5 files changed, 81 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/9339/1
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9339/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9339/1//COMMIT_MSG@10
PS1, Line 10: a decimal, a MULL was returned. In this patch, we change this behavior
NULL
http://gerrit.cloudera.org:8080/#/c/9339/1/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
File fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java:
http://gerrit.cloudera.org:8080/#/c/9339/1/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@85
PS1, Line 85: if (resultOfReverseCast != null &&
It seems weird (at least to me) that LiteralExpr.create would return null instead of NullLiteral, and probably simpler to just make this the case even if errors are raised during the cast.
It also seems strange that we would only hit this with decimal. Don't other casts also raise errors?
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 23:19:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
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
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M tests/query_test/test_decimal_casting.py
5 files changed, 57 insertions(+), 15 deletions(-)
Approvals:
Dan Hecht: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 6
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc@584
PS2, Line 584: ctx->AddWarning("String to Decimal cast underflowed");
> Doesn't that mean that the comment for IMPALA-5014: Part 1 isn't doing what
Also, regarding the warning, it seems legit to have cases where you have strings that have a bunch of fractional digits but you only care about the significant digit for whatever calculation you are doing.
What do other systems do in this case?
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 23:03:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
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 return in 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
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M tests/query_test/test_decimal_casting.py
5 files changed, 81 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/9339/2
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9339/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9339/1//COMMIT_MSG@10
PS1, Line 10: a decimal, a MULL was returned. In this patch, we change this behavior
> NULL
oops, done.
http://gerrit.cloudera.org:8080/#/c/9339/1/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
File fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java:
http://gerrit.cloudera.org:8080/#/c/9339/1/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@85
PS1, Line 85: if (resultOfReverseCast != null &&
> It seems weird (at least to me) that LiteralExpr.create would return null i
No, other casts do not raise errors.
LiteralExpr.create() returns a null when there is an error or a warning according to the comment in the Java file. To me this does not seem weird.
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:05:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
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 return in 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
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M tests/query_test/test_decimal_casting.py
5 files changed, 81 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/9339/3
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 19:46:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc@584
PS2, Line 584: ctx->AddWarning("String to Decimal cast underflowed");
> I guess this case can only happen with DECIMAL_V2=false? Because of:
No, this does not happen only when decimal_v2 is false. (Take a look at the tests that I added).
I don't think this would bee too loud and occur too frequently. I doubt that there are many cases where a user wants to convert a string with more digits after the dot than the scale of the decimal. My guess is in most cases when this warning happens, it means something went wrong, so issuing a warning is ok.
http://gerrit.cloudera.org:8080/#/c/9339/2/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
File testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test:
http://gerrit.cloudera.org:8080/#/c/9339/2/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test@459
PS2, Line 459: decimal_v2=0;
> nit: existing cases use decimal_v2=false/true. Would be good to be consiste
Done
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 22:57:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 5
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 03:29:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc@584
PS2, Line 584: ctx->AddWarning("String to Decimal cast underflowed");
> Right, but given we've made an explicit choice to round, it seems to me tha
Postgres does not issue any warnings. I think then it would make sense to not issue the warning for both decimal v1 and v2. Updated the code.
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 23:31:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
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
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M tests/query_test/test_decimal_casting.py
5 files changed, 81 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/9339/4
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 5
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 23:46:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc@584
PS2, Line 584: ctx->AddWarning("String to Decimal cast underflowed");
I guess this case can only happen with DECIMAL_V2=false? Because of:
IMPALA-5014: Part 1: Round when casting string to decimal.
Maybe clarify with DCHECK(!decimal_v2);
This warning might be very loud. Are we sure it won't occur too frequently?
http://gerrit.cloudera.org:8080/#/c/9339/2/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
File testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test:
http://gerrit.cloudera.org:8080/#/c/9339/2/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test@459
PS2, Line 459: decimal_v2=0;
nit: existing cases use decimal_v2=false/true. Would be good to be consistent to make it easier to grep/read.
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 19:48:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2048/
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 5
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 23:47:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc@584
PS2, Line 584: ctx->AddWarning("String to Decimal cast underflowed");
> Doesn't that mean that the comment for IMPALA-5014: Part 1 isn't doing what
IMPALA-5014: Part 1 changes the behavior so that when then there is an underflow, we round instead of truncating when decimal_v2 is enabled. So that patch is correct. Before this patch, underflows happened silently. After this patch we will issue a warning.
"Underflow" means that not all digits to the right of the dot in the string could fit into the resulting decimal. "Overflow" means that not all digits to the left of the dot could fit into the resulting decimal.
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 23:08:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
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
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M tests/query_test/test_decimal_casting.py
5 files changed, 57 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/9339/5
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 5
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc@584
PS2, Line 584: ctx->AddWarning("String to Decimal cast underflowed");
> IMPALA-5014: Part 1 changes the behavior so that when then there is an unde
Right, but given we've made an explicit choice to round, it seems to me that warning may be unwarranted.
Could you see what happens in the case of e.g. postgres?
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 23:16:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9339 )
Change subject: IMPALA-6405: Error when string to decimal cast overflows
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc@584
PS2, Line 584: ctx->AddWarning("String to Decimal cast underflowed");
> No, this does not happen only when decimal_v2 is false. (Take a look at the
Doesn't that mean that the comment for IMPALA-5014: Part 1 isn't doing what it claims to have done? If we do rounding, then what does it mean to underflow?
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 23:00:38 +0000
Gerrit-HasComments: Yes