Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8404 )
Change subject: IMPALA-5017: Error on decimal overflow ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/8404/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8404/1//COMMIT_MSG@12 PS1, Line 12: In this patch, the beharior is changed so that an error is produced in > typo: beharior should be behavior I'll also accept behaviour ;) http://gerrit.cloudera.org:8080/#/c/8404/1//COMMIT_MSG@27 PS1, Line 27: select avg(dec_38_19) from decimal_tbl I guess these regressions occur regardless of the width of the input decimal, rigth? http://gerrit.cloudera.org:8080/#/c/8404/1/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/8404/1/be/src/exprs/aggregate-functions-ir.cc@413 PS1, Line 413: abs(avg->sum_val16) > DecimalUtil::MAX_UNSCALED_DECIMAL16 - abs(src.val4))) { This isn't correct if sum_val16 and src.val* have opposite sign, is it? http://gerrit.cloudera.org:8080/#/c/8404/1/be/src/exprs/aggregate-functions-ir.cc@420 PS1, Line 420: abs(avg->sum_val16) > DecimalUtil::MAX_UNSCALED_DECIMAL16 - abs(src.val8))) { What do you think about only checking for overflow of the integer type at this point and checking for whether it fits in the decimal type during finalisation? Might be cheaper. We could also probably do a cheaper initial check of sum_val16 versus as constant in the 4 and 8 byte cases since they can't possible overflow unless the sum is already quite large. -- To view, visit http://gerrit.cloudera.org:8080/8404 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id98a92c9a9469ec8cf14e518c741a2dab7053019 Gerrit-Change-Number: 8404 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: John Sherman <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-Comment-Date: Fri, 03 Nov 2017 23:21:08 +0000 Gerrit-HasComments: Yes
