Zach Amsden has posted comments on this change. Change subject: IMPALA-4813: Round on divide and multiply ......................................................................
Patch Set 8: (12 comments) Thanks for the review! 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 Without the result type change, I have been unable to find a meaningful way to test this. 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)? Probably. Our current behavior is more precise. I like it. 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_ I'll double check - this was supposed to be an exact copy of the logic from the Java code 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: That is a better suggestion. Line 212: result += (BitUtil::Sign(value) ^ 1) + 1; > same Done PS8, Line 311: sp > what does "sp" stand for? single precision (not entirely accurate, but...) Let me know if you want me to change this Line 346: DCHECK(r != 0); > I don't understand this comment or this dcheck. Is "precision" referring t It's actually impossible to get a zero value here (value == non-zero value). If we have a remainder (impossible for x == 0), and we didn't need to promote to a higher type, there is no way to have result (r) != 0 because we should have then promoted to a higher type. Since the only problematic case is actually getting a zero result, and we can't get that, we don't need extra bit manipulations for the special case of zero. Line 347: r = BitUtil::IncrementAwayFromZero(r); > given that this turned out to not be generally usable and you have the Sign I don't mind doing that. Line 350: DCHECK(sizeof(RESULT_T) > 8 || abs(r) <= DecimalUtil::MAX_UNSCALED_DECIMAL8); > what does this DCHECK mean? why is 8 significant? Either we have to promote to a RESULT_T that can hold the result, or there is no overflow. Some of the unit tests violated this condition and required changes (int64_t -> int128_t for divide) 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 Sure, but then all the following template conditions will become grotesque: typename std::enable_if(std::is_integral<typename std::decay<T>::type>{} || std::is_same<typename std::decay<T>::type{}, unsigned __int128> || ... Using a template type parameter is better as not only is it more concise, it eliminates many possibilities for errors in the following by giving everything following the same syntax. I don't use enable_if lightly, but in this case it is actually prudent to prevent misuse of the definition on unknown types. PS8, Line 69: positive > non-negative Done Line 72: constexpr static inline T IncrementAwayFromZero(T value) { > but is this worth having anymore given you need to use Sign() in most place Probably not. The discovery of the signed zero underflow bias has probably killed the utility of this utility. -- 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
