Michael Ho has posted comments on this change. Change subject: IMPALA-2020: Add rounding for decimal casts ......................................................................
Patch Set 15: (6 comments) 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* BenchmarkDecimalCast() { Can you please add a sample output like other benchmark functions ? 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. Also may help to add cases which cause overflow due to rounding. Decimal16Value::FromDouble(t3, 0.99999, true, &overflow); Decimal16Value::FromDouble(t3, 0.99999, false, &overflow); PS15, Line 224: Decimal4Value::FromDouble(t4, 0.499999999, true, &overflow); Isn't a duplicate of the test above ? 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 places otherwise. 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); : DCHECK(max_value > 0); // no DCHECK_GT because of int128_t : if (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); : > Sort of. I didn't want to change the original at all in this change though Are there particular cases you have in mind which may break if we use V2's logic for both version ? 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 -- 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: 15 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
