impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4813: Round on divide and multiply
Date Fri, 24 Feb 2017 23:49:30 GMT
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 <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