Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9114 )
Change subject: IMPALA-6429: Fix decimal division ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/9114/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9114/1//COMMIT_MSG@18 PS1, Line 18: dividend. > Will that ever lead to reporting overflow when the final answer does fit in No, I don't think so. I added a test to prove this. http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/exprs/expr-test.cc@2400 PS1, Line 2400: { true, false, 0, 38, 6 }}}, > Suggest also adding another test with full precision decimal(38,3) / decima Done http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/runtime/decimal-value.inline.h@235 PS1, Line 235: return DecimalUtil::SafeMultiply(left, mult, *overflow) + right; > This is going to check *overflow with a branch anyway. Why not hoist the c Actually it's not true that SafeMultiply will check for overflow with a branch (at least in the release build). SafeMultiply() is just a multiplication followed by a DCHECK. I think it's best to leave it as is. What do you think? http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/runtime/decimal-value.inline.h@288 PS1, Line 288: return DecimalUtil::SafeMultiply(left, mult, *overflow) + right; > Same comment applies. Same response as above. http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/runtime/decimal-value.inline.h@457 PS1, Line 457: bool may_overflow = scale_by > 38; > You could add a leading zero check as well, but that requires knowing the m I made pretty large modifications to how this is done in the latest patch. We check for overflow ONLY by looking at the leading zeros. Turns out this is sufficient. http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/util/decimal-util.h File be/src/util/decimal-util.h: http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/util/decimal-util.h@67 PS1, Line 67: if (UNLIKELY(result / multiplier != v)) *overflow = true; > You can infer !may_overflow from overflow == nullptr, and this should inlin Rewrote the patch quite significantly, so this does not quite apply. In the latest patch, this check is not necessary any more, so it should be faster. -- To view, visit http://gerrit.cloudera.org:8080/9114 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibd1075d9c78986cd975dd29c1125d71ba6560c23 Gerrit-Change-Number: 9114 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-Comment-Date: Tue, 30 Jan 2018 23:02:31 +0000 Gerrit-HasComments: Yes
