Michael Ho has posted comments on this change. Change subject: IMPALA-2020: Add rounding for decimal casts ......................................................................
Patch Set 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/5951/10/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: Line 113: if (divisor == 1) { > Things like dcheck, the divide, modulus and rounding are completely unneces I guess most of those would be constant folded away by the compiler once we propagate 'scale' as constant. divisor would be known at that point and it would just constant fold remainder to 0. I guess it may not be able to remove abs(remainder) but other than that it's mostly the same as the code above. PS10, Line 117: const T remainder = v % divisor; > No, but I was worried that moving the computation further away from the div May be it's more important to look at the cross-compiled IR file output as that's gonna be the version we'll use. PS10, Line 127: typename RESULT_T::underlying_type_t > I can only get rid of this one, not the other, so it seems more confusing j It's applicable to the next line too, right ? The following looks easier to parse IMHO but I guess some would say it's personal preference: *overflow = result > std::numeric_limits<result_type>::max() || result < std::numeric_limits<result_type>::min(); -- To view, visit http://gerrit.cloudera.org:8080/5951 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810 Gerrit-PatchSet: 10 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
