Taras Bobrovytsky 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 I'm checking it with negative numbers here. In the if condition it was with the positive numbers. 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 Done 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() implementa Might be worth experimenting with. Added a TODO comment. 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 :) 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? Good catch. I have no idea. I'm not sure how it even got compiled. It looks like it doesn't get called by anything, so it's dead code. I removed it. -- 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 23:04:01 +0000 Gerrit-HasComments: Yes
