Zach Amsden has posted comments on this change. Change subject: IMPALA-4813: Round on divide and multiply ......................................................................
Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/6132/2/be/src/exprs/decimal-operators-ir.cc File be/src/exprs/decimal-operators-ir.cc: PS2, Line 729: \ > ROUND && ? I thought I already got rid of this, it's not needed. http://gerrit.cloudera.org:8080/#/c/6132/5/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: PS5, Line 194: namespace detail { > Why new namespace ? To avoid name collision ? Yeah, even though this is "inline.h", this is still a private implementation detail. PS5, Line 203: DCHECK(multiplier > 1); > DCHECK(multiplier > 0 && multiplier % 2 == 0); We still want multiplier > 1, since delta_scale > 0 PS5, Line 205: RESULT_T result = value / multiplier; > Please consider hoisting this out of the loop so we don't need the else sta Done PS5, Line 208: a multiple of two. > Should there be a DCHECK for this ? See comment above. Done PS5, Line 249: // Check overflow again after rounding since +/-1 could cause decimal overflow : if (result_precision == ColumnType::MAX_PRECISION) { : *overflow |= abs(result) > DecimalUtil::MAX_UNSCALED_DECIMAL16; > Should this be inside the if-stmt above ? I don't see any rounding outside Done PS5, Line 285: // Check overflow again after rounding since +/-1 could cause decimal overflow : if (result_precision == ColumnType::MAX_PRECISION) { : *overflow |= abs(result) > DecimalUtil::MAX_UNSCALED_DECIMAL16; > Should this be inside the if-stmt above ? I don't see any rounding outside Done PS5, Line 314: if (round) { > Should we skip this if *overflow is true ? probably not worth it. If things can inline, we may start computing x % y before overflow's value is known, so we may get better code. PS5, Line 316: free : // free > duplicated "free" Done -- To view, visit http://gerrit.cloudera.org:8080/6132 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie6bfcbe37555b74598d409c6f84f06b0ae5c4312 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
