Zach Amsden has posted comments on this change. Change subject: IMPALA-2020: Add rounding for decimal casts ......................................................................
Patch Set 9: (11 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 ? will cull http://gerrit.cloudera.org:8080/#/c/5951/9/be/src/exprs/decimal-operators-ir.cc File be/src/exprs/decimal-operators-ir.cc: PS9, Line 303: round > Yeah, I got that. What I'm saying is why not remove the 'round' paramter s Killing it with fire http://gerrit.cloudera.org:8080/#/c/5951/9/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 1503: { true, 0, 3, 0 }}}, > okay. i missed that the scale=1 case covers that. It doesn't hurt to do anyways, and does apply some pressure to other corners. Would probably not hurt to add + 0.55 tests as well. http://gerrit.cloudera.org:8080/#/c/5951/10/be/src/runtime/decimal-value.h File be/src/runtime/decimal-value.h: PS10, Line 237: /// Returns an approximate double for this decimal. > long line Will fix http://gerrit.cloudera.org:8080/#/c/5951/9/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: PS9, Line 43: precise > sure, fixing in a separate change sounds fine. If there's no jira for that Freakily enough, 100000000000000000000000000000000000000 has exactly 53 significant digits in binary. So it should be represented exactly as a double precision floating point value. This means conversions from float to decimal(38, 38) should be well behaved. 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: Sort of. I didn't want to change the original at all in this change though. We can do this if we can prove the results will be identical. PS10, Line 108: typename RESULT_T::underlying_type_t > May help to add a comment that underlying_type_t is defined only for the *I Done Line 113: if (divisor == 1) { > If it's for optimization, constant propagation in codegen would have reaped Things like dcheck, the divide, modulus and rounding are completely unnecessary in this case. PS10, Line 117: const T remainder = v % divisor; > Do we not need to compute this if round is false ? No, but I was worried that moving the computation further away from the divide would cause the compiler to do the divide twice. We'll try it and see. I haven't even gotten to looking at the release binary yet. PS10, Line 119: DCHECK( > DCHECK_EQ Can't do it or GCC goes completely insane as it can't find ostream operators for int128_t. PS10, Line 127: typename RESULT_T::underlying_type_t > May be consider doing a typedef typename RESULT_T::underlying_type_t result I can only get rid of this one, not the other, so it seems more confusing just to do that for only one use. -- 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: 9 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
