Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9114 )
Change subject: IMPALA-6429: Fix decimal division ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/exprs/expr-test.cc@2400 PS1, Line 2400: { true, false, 0, 38, 6 }}}, Suggest also adding another test with full precision decimal(38,3) / decimal(1,0), which will overflow in V2 since the result can't fit in decimal(38,6). Even the value 1.0 will work as a divisor for this. http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/runtime/decimal-value.inline.h@235 PS1, Line 235: return DecimalUtil::SafeMultiply(left, mult, *overflow) + right; This is going to check *overflow with a branch anyway. Why not hoist the check of *overflow to this function? Then if (UNLIKELY(*overflow == true,)) return garbage, and always call DecimalUtil::SafeMultiply with false. http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/runtime/decimal-value.inline.h@288 PS1, Line 288: return DecimalUtil::SafeMultiply(left, mult, *overflow) + right; Same comment applies. http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/runtime/decimal-value.inline.h@457 PS1, Line 457: bool may_overflow = scale_by > 38; You could add a leading zero check as well, but that requires knowing the maximum value of scale_by. This adds one more corner case that relies on the exact formula for result_scale, and that makes me uncomfortable. In the future, we may want to improve decimal results by allowing the query to specify the output scale (as a cast, which gets pushed down into result_scale), and doing that is already a bit dangerous as I think there are other places that rely on the result scale formulas being predefined. http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/util/decimal-util.h File be/src/util/decimal-util.h: http://gerrit.cloudera.org:8080/#/c/9114/1/be/src/util/decimal-util.h@67 PS1, Line 67: if (UNLIKELY(result / multiplier != v)) *overflow = true; You can infer !may_overflow from overflow == nullptr, and this should inline just as well in cases where constant nullptr is passed. (Edit - I see the way you are using it, this isn't quite true - I think if you adopt my suggestions for Add / SubtractLarge, then this may follow). Also, this is likely expensive for int256_t. I don't think there is a practical way to make that faster without direct access to the underlying type (to get the number of leading zeroes). -- To view, visit http://gerrit.cloudera.org:8080/9114 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibd1075d9c78986cd975dd29c1125d71ba6560c23 Gerrit-Change-Number: 9114 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-Comment-Date: Wed, 24 Jan 2018 18:16:28 +0000 Gerrit-HasComments: Yes
