Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9114 )
Change subject: IMPALA-6429: Fix decimal division ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/9114/3/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/9114/3/be/src/exprs/expr-test.cc@2877 PS3, Line 2877: EXPECT_TRUE((-scaled_up_dividend) / scale_multiplier != I'm not sure how valuable this EXPECT_TRUE is - haven't we already verified that with the if condition? http://gerrit.cloudera.org:8080/#/c/9114/3/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: http://gerrit.cloudera.org:8080/#/c/9114/3/be/src/runtime/decimal-value.inline.h@161 PS3, Line 161: return floor_log2[scale_by] + 1; Why not add the +1 into the floor function? Then you can actually omit it for the case of zero, where the formula is conservative (no increase in bits multiplying by 1). http://gerrit.cloudera.org:8080/#/c/9114/3/be/src/runtime/decimal-value.inline.h@175 PS3, Line 175: int num_occupied = 128 - BitUtil::CountLeadingZeros<int128_t>(abs(num)); An optimization idea for later. Curious how efficient the abs() implementation is for int128_t. Given that we have to do a lot of these abs() transformations, might it make sense for us to do that up front for large multiply and divide, do everything in unsigned math, and correct the result after? This also simplifies getting the rounding correct. http://gerrit.cloudera.org:8080/#/c/9114/3/be/src/util/decimal-util.h File be/src/util/decimal-util.h: http://gerrit.cloudera.org:8080/#/c/9114/3/be/src/util/decimal-util.h@54 PS3, Line 54: /// TODO: do we need to handle overflow here or at a higher abstraction. TODO looks to be now done :) http://gerrit.cloudera.org:8080/#/c/9114/3/be/src/util/decimal-util.h@58 PS3, Line 58: return MultiplyByScale(v, t.scale, false, nullptr); Which function is this actually calling? -- 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: 3 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: Wed, 31 Jan 2018 20:00:33 +0000 Gerrit-HasComments: Yes
