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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
