Dan Hecht has posted comments on this change. Change subject: IMPALA-4813: Round on divide and multiply ......................................................................
Patch Set 2: (12 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 682: ROUND I don't see why we need this. it happens that our Add() et al result types don't need rounding, but that's because of how we choose their result type. i.e. these operations already handle rounding enabled and disabled correctly (since the answer is the same, and they have the dchceks in place already for not losing scale). But that's a detail that the decimal-value code cares about, not this code. also see my comments there. http://gerrit.cloudera.org:8080/#/c/6132/2/be/src/runtime/decimal-value.h File be/src/runtime/decimal-value.h: Line 124: int result_scale, bool* overflow) const { where are these variants used? PS2, Line 155: /* round */ true why? PS2, Line 169: /* round */ true same http://gerrit.cloudera.org:8080/#/c/6132/2/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: Line 153: DCHECK_EQ(round, false); the DCHECK_EQ(result_scale, std::max(this_scale, other_scale)); means that we never lose scale and so rounding is a no-op, so why not allow the caller to pass the actual value of 'round'? One day, we may change the result type of Add(), and in some cases lose scale, in which case round will no longer be a no-op and at that point the dcheck at line 152 would no longer hold telling us that this code needs to be updated. Line 176: DCHECK_EQ(round, false); same Line 198: // Helper because int128_t is not part of std::numeric_limits explain what this does PS2, Line 200: shift_width ShiftWidth though the name doesn't really tell you what it does. maybe the subroutine should be something like: T IncrementAwayZero(T value) { int shift_by = ... stuff here... return value + (1 | value >> shift_by); } and then you can use that all over. Line 294: result = scaled_down; how about factoring this out into subroutine rather than duplicating? PS2, Line 328: 127 should this be 255? (or rather, shift_width()?) PS2, Line 340: 127 shift_width()? Line 353: DCHECK_EQ(round, false); same -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <zams...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho Gerrit-HasComments: Yes
