Taras Bobrovytsky 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 > I'll also accept behaviour ;) Done 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 decima Correct. Added another benchmark to illustrate this. 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? That's true. Fixed and added a test case. 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 t 1. Are you suggesting to check for something like abs(avg->sum_val16) > 2**128 - abs(src.val8))) ? I don't really see why it is faster to check against 2**128 instead of against MAX_UNSCALED_DECIMAL. 2. What do you mean by initial check? Where would this check be done? Something like (decimal_v2 && sum_val16 > 2**32 && abs(avg->sum_val16) > MAX_UNSCALED_DECIMAL - abs(src.val8)))? -- 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: Tue, 07 Nov 2017 21:59:10 +0000 Gerrit-HasComments: Yes
