Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/8833 )
Change subject: IMPALA-6300: Fix decimal modulo overflow ...................................................................... Patch Set 1: (4 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? Either way, I don't object to it (and I know how much fun merging changes in here can be). 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? 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 the DCHECK? http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h@261 PS1, Line 261: #ifdef DEBUG > Is there some way we can avoid having different control flow on release and Then you can omit the #ifdef here and just call CheckMultiply(left, mult) if (!*overflow) -- 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 <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-Comment-Date: Fri, 15 Dec 2017 21:52:14 +0000 Gerrit-HasComments: Yes
