Zach Amsden has posted comments on this change. Change subject: IMPALA-4813: Round on divide and multiply ......................................................................
Patch Set 2: (4 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 > GetConstFnAttr() is just as free as a constant 'false'. The call goes away Yeah, I guess that makes sense. Is there another pass that removes unused constants after substitution? 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 { > If they are only used by tests, I agree killing them would be nice. Attempting to do so 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); > but what i'm saying is we should actually pass the correct value of 'round' Done Line 235: if (UNLIKELY(delta_scale != 0)) { Multiply appears to be buggy. If we ever need to chop the scale, then results that shouldn't actually overflow after reduction can end up overflowing anyway. For example, this query overflows despite the fact that the result obviously does not: select typeof(t.v), t.v from (select cast(0.001 as decimal(9,4)) * cast(.1 as decimal(38,38))as v) t; -- 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
