Dan Hecht has posted comments on this change. Change subject: IMPALA-2020: Add rounding for decimal casts ......................................................................
Patch Set 9: (10 comments) How about augmenting test_decimal_casts.py to also test these cases? (You'll probably want Michael's change). As we discussed, on a release build, let's do a quick microbenchmark to verify perf with these three points: - Before this change. - After this change with v2=false - After this change And we want the first two to match (no regression with v2=false). 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 the "else" clause, *or* leave the else clause here but don't pass round. Why do we have both? I think you concluded that the else-clause was better because of the change in overflow behavior, so that's fine. but then we don't need 'round' (we never pass false). 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. Line 1460: { true, 0, 4, 0 }}}, let's include tests at the boundary of the 'int' ranges to exercise the new overflow check you added. let's also test the other int types (bigint, tinyint, smallint) Line 1503: { true, 0, 3, 0 }}}, what happens on the "exact" boundary, i.e. +/- 0.5? why not test that case as well? 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 as i mentioned in decimal-operators-ir.cc, let's either keep the else-clause there or use the 'round' bool here, but not both. PS9, Line 234: RESULT_T this should be explained in the comment as well. 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 current implementation in some cases? PS9, Line 110: auto we generally use auto only when it makes the code more readable (because, say, the type is really long like for an iterator). Here, the type can just be T, right? PS9, Line 111: auto same PS9, Line 119: DCHECK DCHECK_EQ (it'll print both sides on failure). -- 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
