Michael Ho 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 && ? 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 ? PS5, Line 203: DCHECK(multiplier > 1); DCHECK(multiplier > 0 && multiplier % 2 == 0); PS5, Line 205: RESULT_T result = value / multiplier; Please consider hoisting this out of the loop so we don't need the else statement. PS5, Line 208: a multiple of two. Should there be a DCHECK for this ? See comment above. 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 of that. 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 of that. PS5, Line 314: if (round) { Should we skip this if *overflow is true ? PS5, Line 316: free : // free duplicated "free" -- 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
