Zach Amsden has posted comments on this change. Change subject: IMPALA-2020: Add rounding for decimal casts ......................................................................
Patch Set 9: (10 comments) 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 > what i meant earlier was we should either pass 'round' here and get rid of Otherwise this is a behavior change. dv.ToInt detects overflow. to_type(dv.whole_part(scale)) does not. http://gerrit.cloudera.org:8080/#/c/5951/9/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: PS9, Line 1442: { false, 9999999900, 10, 2 }}}, > you can delete this line since the results match. Done Line 1460: { true, 0, 4, 0 }}}, > let's include tests at the boundary of the 'int' ranges to exercise the new will do Line 1503: { true, 0, 3, 0 }}}, > what happens on the "exact" boundary, i.e. +/- 0.5? why not test that case due to scaling, we get coverage anyway as 0.45 ends in 5. I can add it, but I don't think it gives any better coverage. http://gerrit.cloudera.org:8080/#/c/5951/9/be/src/runtime/decimal-value.h File be/src/runtime/decimal-value.h: PS9, Line 232: Optionally rounds to the nearest integer, : /// defined as half round away from zero. > this contradicts the first sentence which says rounding always happens. but has to wait for V1 to be deleted PS9, Line 234: RESULT_T > this should be explained in the comment as well. Done 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 > what is the consequence of that? do we get the wrong answer with the curren Well, there aren't enough bits in the floating point multiplier to fully represent the larger scale multipliers. We have 10^15 being around a 53-bit number, which is what we have for our mantissa and we get another 10 or so bits of representation for free because of the trailing zeros in the representation of 10 = 5 * 2, so after scales of somewhere less than 25, DecimalUtil::GetScaleMultiplier<double> will only be an approximation of the actual number. If it rounds to nearest, then values below (10**scale) may overflow when multiplied by this number. Note both V1 and V2 code have this problem - the overflow check in V1 logic may miss a wraparound to a very low number. The V2 code does not have this problem, but instead may have the problem of false overflow because of an inaccurate double representation of large scales. I'd rather fix this issue in a separate change. PS9, Line 110: auto > we generally use auto only when it makes the code more readable (because, s Done PS9, Line 111: auto > same Done PS9, Line 119: DCHECK > DCHECK_EQ (it'll print both sides on failure). Done -- 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: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
