Michael Ho has posted comments on this change. Change subject: IMPALA-2020: Add rounding for decimal casts ......................................................................
Patch Set 10: (9 comments) http://gerrit.cloudera.org:8080/#/c/5951/10//COMMIT_MSG Commit Message: PS10, Line 10: Still writing tests, : but this is ready for human eyes to look at. Not needed in the commit message anymore, right ? http://gerrit.cloudera.org:8080/#/c/5951/5/be/src/exprs/literal.cc File be/src/exprs/literal.cc: Line 185: value_.decimal4_val = Decimal4Value::FromDouble(type, v, true, &overflow); > This functionality (constructing Literals given a ColumnType) is only used Shouldn't this still be dependent on the behavior we want to test ? We test both the v1 vs v2 behavior in the BE test. May consider taking an extra argument (bool round) ? http://gerrit.cloudera.org:8080/#/c/5951/10/be/src/runtime/decimal-value.h File be/src/runtime/decimal-value.h: PS10, Line 237: inline typename RESULT_T::underlying_type_t ToInt(int scale, bool round, bool* overflow) const; long line http://gerrit.cloudera.org:8080/#/c/5951/10/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: PS10, Line 38: if (round) { : // Decimal V2 behavior : : // Multiply the double by the scale. : // : // TODO: this could be made more precise by doing the computation in : // 64 bit with 128 bit immediate result instead of doing an additional : // multiply in 53-bit double precision floating point : : d *= DecimalUtil::GetScaleMultiplier<double>(scale); : d = std::round(d); : const T max_value = DecimalUtil::GetScaleMultiplier<T>(precision); : if (abs(d) >= max_value) { : *overflow = true; : return DecimalValue(); : } : : // Return the rounded integer part. : return DecimalValue(static_cast<T>(d)); : } else { : // TODO: IMPALA-4924: remove DECIMAL V1 code : : // Check overflow. : const T max_value = DecimalUtil::GetScaleMultiplier<T>(precision - scale); : if (abs(d) >= max_value) { : *overflow = true; : return DecimalValue(); : } : : // Multiply the double by the scale. : d *= DecimalUtil::GetScaleMultiplier<double>(scale); : : // Truncate and just take the integer part. : return DecimalValue(static_cast<T>(d)); : } Appears to me that we should be able to share the code for the most part: d *= DecimalUtil::GetScaleMultiplier<double>(scale); if (round) d = std::round(d); const T max_value = DecimalUtil::GetScaleMultiplier<T>(precison); if (abs(d) >= max_value) { ... } return DecimalValue(static_cast<T>(d)); PS10, Line 108: typename RESULT_T::underlying_type_t May help to add a comment that underlying_type_t is defined only for the *IntType in udf.h Line 113: if (divisor == 1) { > what's this for? is it an optimization for scale==0 case? If it's for optimization, constant propagation in codegen would have reaped most of the benefit as this is inlined into cross-compiled code. PS10, Line 117: const T remainder = v % divisor; Do we not need to compute this if round is false ? PS10, Line 119: DCHECK( DCHECK_EQ PS10, Line 127: typename RESULT_T::underlying_type_t May be consider doing a typedef typename RESULT_T::underlying_type_t result_type; inside here for ease of reading. -- 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 <zams...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-HasComments: Yes
