impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4813: Round on divide and multiply
Date Fri, 24 Feb 2017 22:06:13 GMT
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 <zamsden@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message