impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4936 and IMPALA-4915: Fix decimal overflow test
Date Wed, 22 Feb 2017 06:27:59 GMT
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 <zamsden@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message