You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zach Amsden (Code Review)" <ge...@cloudera.org> on 2017/02/18 04:37:22 UTC
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Zach Amsden has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6068
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
The root cause of both behaviors turned out to be the same - a
missing std:: caused the wrong abs() function to be used. Due
to details of IEEE floating point representation, this actually
masked another bug, as NaN is often represented as all 1-bits,
which fails the overflow test. Since the implicit conversion to
int lost precision, we ended up storing large numbers that don't
actually represent valid decimal numbers in the range when the
value happened to be +/- Infinity. This caused the rendering
back to ASCII characters to go awry, but is otherwise harmless.
Testing: updated expr test with correctly fixed test cases.
For some reason, it seemed the test passed before that, so it
is possible that it isn't correctly validating some of the non
decimal V2 clauses. Will investigate further.
Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
---
M be/src/exprs/expr-test.cc
M be/src/runtime/decimal-value.inline.h
2 files changed, 7 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/6068/1
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6068/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
PS2, Line 1397: true
Sorry, I may be missing something. Why is this NULL ?
http://gerrit.cloudera.org:8080/#/c/6068/2/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:
PS2, Line 83: abs
I think we should be consistent and use std::abs() at least in this entire file if not the entire code base.
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6068/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 1620: {{ true, 0, 9, 0 }}},
May help to add test cases which can fit in a double but still overflow when casting to decimal such as:
select ("555....5" as DECIMAL(38,0)); //39 decimal places value
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6068/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
PS2, Line 1397: true
> I am missing something too... was the test broken before?
I filed JIRA-4964 to investigate. Test was broken before and seems to have uncovered a bug with modulus.
http://gerrit.cloudera.org:8080/#/c/6068/2/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:
PS2, Line 83: abs
> I think we should be consistent and use std::abs() at least in this entire
I tried that and it actually turned out to be a bug. The places where std::abs and abs are correct turned out to be a lot more subtle than expected. std::abs selects the correct version for floating point but the wrong version for integers as abs(INT_MIN) is undefined.
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has uploaded a new patch set (#2).
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
The root cause of both behaviors turned out to be the same - a
missing std:: caused the wrong abs() function to be used. Due
to details of IEEE floating point representation, this actually
masked another bug, as NaN is often represented as all 1-bits,
which fails the overflow test. Since the implicit conversion to
int lost precision, we ended up storing large numbers that don't
actually represent valid decimal numbers in the range when the
value happened to be +/- Infinity. This caused the rendering
back to ASCII characters to go awry, but is otherwise harmless.
Testing: updated expr test with correctly fixed test cases.
For some reason, it seemed the test passed before that, and I
found that it wasn't correctly validating some of the NULL cases.
Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
---
M be/src/exprs/expr-test.cc
M be/src/runtime/decimal-value.inline.h
2 files changed, 14 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/6068/2
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6068/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
PS2, Line 1397: true
> I am missing something too... was the test broken before?
yes, the test was broken before. we do return NULL in these cases. Technically, we should return 0.998 in both but due to how we implement MOD, we overflow during the calculation. That's a similar problem to IMPALA-4939, but I think less important and severe.
Line 1635: TestIsNotNull(c.expr, type);
> I added this because I noticed the non-NULL test cases were passing even wh
thanks for finding that. we should probably add
EXPECT_EQ(result, StringParser::PARSE_SUCCESS);
to the end of TestDecimalValue(). What's happening is the decimal string parser is failing (when parsing "NULL"), but when it does that it happens to return a DecimalValue with value()=0.
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6068/1//COMMIT_MSG
Commit Message:
Line 22: decimal V2 clauses. Will investigate further.
I think this probably should be fixed in this commit.
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
Patch Set 2:
(3 comments)
I'm folding this into the cast change anyway.
http://gerrit.cloudera.org:8080/#/c/6068/1//COMMIT_MSG
Commit Message:
Line 22:
> I think this probably should be fixed in this commit.
This was a real problem, found it.
http://gerrit.cloudera.org:8080/#/c/6068/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
PS2, Line 1397: true
> Sorry, I may be missing something. Why is this NULL ?
I am missing something too... was the test broken before?
Line 1635: TestIsNotNull(c.expr, type);
I added this because I noticed the non-NULL test cases were passing even when the value happened to match, which was the case for some zero values. Looks like something is broken with modulus.
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6068/2/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:
PS2, Line 83: abs
> I'm confused: how is std::abs wrong here for integers? int64_t d is never a
std::abs is only wrong for integers outside of the decimal range, in particular INT_MIN, but here INT_MIN is a valid input. To not have undefined behavior, this test really should be:
if (d > max_value || d < -max_value)
What I'm saying is that blanket replacement of abs with std::abs isn't going to solve everything, with 2's complement, both abs() and std::abs() have undefined (or unspecified) behavior.
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6068/2/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:
PS2, Line 83: abs
> I tried that and it actually turned out to be a bug. The places where std:
I'm confused: how is std::abs wrong here for integers? int64_t d is never and int on x86-64 linux, right? Also, isn't C's global-namespace abs UB on INT_MIN?
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
Patch Set 2:
since you folded this in to the other change, please abandon this change so people don't review it more.
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has abandoned this change.
Change subject: IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
......................................................................
Abandoned
Rolled this into casting change
--
To view, visit http://gerrit.cloudera.org:8080/6068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I8de7440a585c1d3d937fcbb435b9ead77e7b5a63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>