Dan Hecht has posted comments on this change. Change subject: IMPALA-2020: Add rounding for decimal casts ......................................................................
Patch Set 11: (5 comments) > I have not updated the Python test yet, as that would require a > rebase. Python test and benchmark will come in next patch set, > this is just to give a clean slate for review. Thanks. Just some minor comments on the code. I think the benchmark is higher priority than the python test since we already have pretty good functional test coverage and so less likely to be surprised, whereas there could be a perf regression. We could even do the python test as a follow on commit if that is a fair amount of work (perhaps even after taking care of divide rounding). http://gerrit.cloudera.org:8080/#/c/5951/11/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: PS11, Line 1526: power(10, 3) - power(10, -1) at some point in the future, we may implement a decimal variant of POWER(), in which case these test cases would no longer exercise the CAST(double as decimal) path. So, how about either adding a check: // The CAST FLOAT -> DECIMAL test cases rely on power() returning // a double. If that changes, those test cases need to be updated. TestStringValue("typeOf(power(10,1))", "DOUBLE"); Or, we could add the explicit cast to double/float now. Line 1601: {{ false, -999, 3, 0 }}}, there all do DOUBLE -> DECIMAL. How about adding some FLOAT -> DECIMAL cases as well? http://gerrit.cloudera.org:8080/#/c/5951/11/be/src/runtime/decimal-value.h File be/src/runtime/decimal-value.h: PS11, Line 232: Optionally delete "Optionally" since it's not optional. PS11, Line 233: otherwise the result is truncated. delete since this doesn't do truncation http://gerrit.cloudera.org:8080/#/c/5951/11/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: PS11, Line 45: mantissaa mantissa -- 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: 11 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
