Dan Hecht has posted comments on this change. Change subject: IMPALA-4813: Round on divide and multiply ......................................................................
Patch Set 8: (12 comments) http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 1339: { "1.23 * cast(1 as decimal(20,3))", {{ false, 123000, 23, 5 }}}, i thought you said there were some multiplies you could do to exercise the multiply rounding path? Line 1408: // N.B. - Google and python both insist that 999999.998 / 999 is 1001.000999 doesn't that just mean they use a scale at 6 (and round)? http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/runtime/decimal-test.cc File be/src/runtime/decimal-test.cc: Line 635: result_scale); this doesn't look correct when t1.precision - t1.scale + t2.scale + result_scale > 38. In that case, we reduce the scale (but always preserve at least 6). http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: Line 114: result += (BitUtil::Sign(v) ^ 1) + 1; why is this not just: result += BitUtil::Sign(v); ? Line 212: result += (BitUtil::Sign(value) ^ 1) + 1; same PS8, Line 311: sp what does "sp" stand for? Line 346: DCHECK(r != 0); I don't understand this comment or this dcheck. Is "precision" referring to decimal precision? And precision of what? and what do you mean by "always have a value"? non-negative value? and why is this case fundamentally different than the sizeof(T) == 16 case? it doesn't seem true that rounding is needed if r != 0, but if r == 0, then rounding is never needed... Line 347: r = BitUtil::IncrementAwayFromZero(r); given that this turned out to not be generally usable and you have the Sign() abstraction, how about getting rid of this and just using: r += Sign(r); here? Line 350: DCHECK(sizeof(RESULT_T) > 8 || abs(r) <= DecimalUtil::MAX_UNSCALED_DECIMAL8); what does this DCHECK mean? why is 8 significant? http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/util/bit-util.h File be/src/util/bit-util.h: Line 52: std::is_same<CVR_REMOVED, __int128>{}, int>::type = 0> can CVR_REMOVED be defined local to UnsignedWidth(), or does that not work for some reason? PS8, Line 69: positive non-negative Line 72: constexpr static inline T IncrementAwayFromZero(T value) { but is this worth having anymore given you need to use Sign() in most places, and that using this is the wrong thing to do when there are fractional parts? -- 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: 8 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
