Dan Hecht has posted comments on this change. Change subject: IMPALA-4813: Round on divide and multiply ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/6132/2/be/src/exprs/decimal-operators-ir.cc File be/src/exprs/decimal-operators-ir.cc: PS2, Line 682: ROUND > Multiply gets defined using this macro, and multiply needs round. Otherwis GetConstFnAttr() is just as free as a constant 'false'. The call goes away and is replaced by a constant when generating the code. http://gerrit.cloudera.org:8080/#/c/6132/2/be/src/runtime/decimal-value.h File be/src/runtime/decimal-value.h: Line 124: int result_scale, bool* overflow) const { > Testing code, called from be/src/runtime/decimal-test.cc, which forges its If they are only used by tests, I agree killing them would be nice. http://gerrit.cloudera.org:8080/#/c/6132/2/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: Line 153: DCHECK_EQ(round, false); > I'm not opposed to removing these. I inserted them mostly to make sure I g but what i'm saying is we should actually pass the correct value of 'round', so the dcheck isn't valid. It happens that given our current result type choices, rounding for these requires no changes. -- To view, visit http://gerrit.cloudera.org:8080/6132 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie6bfcbe37555b74598d409c6f84f06b0ae5c4312 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <zams...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-HasComments: Yes
