Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/8833 )
Change subject: IMPALA-6300: Fix decimal modulo overflow ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/exprs/expr-test.cc@1574 PS1, Line 1574: {{ false, false, 8892800, 12, 2 }}}, > Not sure where this came from, but okay. Did it leak in from another diff? Oh, it's a random bug I ran into in the code that I've just written when developing this patch. (All tests passed, but there was still a bug, and this test reproduces it, so I thought might as well add it). http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/exprs/expr-test.cc@2479 PS1, Line 2479: { "cast(11111 as decimal(6,1)) % cast(2 as decimal(37,35))", > Did this one overflow before your change? Yes it did (I just tried it to confirm) http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h@145 PS1, Line 145: inline T SafeMultiply(T a, T b) { > How about making this void `CheckMultiply(T a, T b)` and then just doing th Took your most recent suggestion, and left the way it is in patch 2. http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h@190 PS1, Line 190: int y_lz = BitUtil::CountLeadingZeros<T>(abs(y)); > This was counting in 64 bit all the time before, right? Ouch because it wa I guess it depended on the output of the abs() function. And I think the output of abs() is the same type as the input. So it was still probably doing the right thing. It's better to make it explicit though, which is what I'm doing here. http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h@261 PS1, Line 261: #ifdef DEBUG > Then you can omit the #ifdef here and just call CheckMultiply(left, mult) i Left it unchanged after patch 2. -- To view, visit http://gerrit.cloudera.org:8080/8833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27c7f25f68353c19c315e1639311ec06b2dea686 Gerrit-Change-Number: 8833 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-Comment-Date: Fri, 15 Dec 2017 22:24:49 +0000 Gerrit-HasComments: Yes
