Zach Amsden has posted comments on this change. Change subject: IMPALA-2020: Add rounding for decimal casts ......................................................................
Patch Set 12: (8 comments) http://gerrit.cloudera.org:8080/#/c/5951/15//COMMIT_MSG Commit Message: Line 122 > Sure, we can try to get this back in follow on commits. Will check http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: PS15, Line 218: BENCHMARK("strncmp1", "'abcdefghi > Can you please add a sample output like other benchmark functions ? I can't - with these changes, this is in a slightly better state, but the benchmark still doesn't run, it segfaults almost immediately due to a NULL descriptor table. After spending the better part of a day attempting to fix it, I've had to shelve this for now. Considering that there are serious issues running this through codegen anyway, I'm not sure what to do with these changes. http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/runtime/decimal-test.cc File be/src/runtime/decimal-test.cc: PS15, Line 169: -.1 > -0.1. Done PS15, Line 224: Decimal4Value::FromDouble(t4, 0.499999999, true, &overflow); > Isn't a duplicate of the test above ? This was supposed to be negative :) http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/runtime/decimal-value.h File be/src/runtime/decimal-value.h: PS15, Line 52: truncating digits that : /// cannot be represented or rounding if desired > rounding to the closest integer if 'round' is true. Truncate the decimal pl Done 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. : // : // Some quick research shows us that 10**38 in binary has exactly 53 : // significant nonzero bits. Coincidentally, double precision floating : // point gives us 53 bits for the mantissa. TODO: validate that the : // values produced by the template are both correct, constant and that : // the multiply does not lose precision. : : d *= DecimalUtil::GetScaleMultiplier<double>(scale); : d = std::round(d); : const T max_value = DecimalUtil::GetScaleMultiplier<T>(precision); : if (max_value < 0 || UNLIKELY(std::isnan(d)) || UNLIKELY(std::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); : : > Are there particular cases you have in mind which may break if we use V2's None I can come up with right now, but this was before I noticed the bit pattern for 10^37 fits exactly in a a double. Now I'm reasonably sure it is safe. http://gerrit.cloudera.org:8080/#/c/5951/12/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: PS12, Line 52: max_value < 0 > Yes, if it's not possible (i.e. only happens due to a software bug somewher Done http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: PS15, Line 132: > nit: long line fixed -- 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: 12 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
