Zach Amsden has posted comments on this change.

Change subject: IMPALA-4813: Round on divide and multiply
......................................................................


Patch Set 2:

(12 comments)

Thanks for the review!

http://gerrit.cloudera.org:8080/#/c/6132/2//COMMIT_MSG
Commit Message:

Line 44: | 46178777464.523809523847285 | 61076151920731.010714285714202    |
> why are these results not the same except for the least significant digit? 
This is the sum over many divisions, so you are seeing the compound error of 
truncation.


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
> I don't see why we need this. it happens that our Add() et al result types 
Multiply gets defined using this macro, and multiply needs round.  Otherwise, 
we don't need an extra call to GetConstFnAttr to fetch an unused detail.


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 {
> where are these variants used?
Testing code, called from be/src/runtime/decimal-test.cc, which forges its own 
"column types" without having any actual columns or FunctionContext from which 
to extract the scale and precision.

I'd be extremely happy to kill these and move them into the actual test instead 
of littering all over this API, which because of the unused round parameter, 
already is getting a bit ugly and overloaded.


PS2, Line 155: /* round */ true
> why?
So the decimal test always rounds :)  More precision as a result.


PS2, Line 169: /* round */ true
> same
Same reason.


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);
> the DCHECK_EQ(result_scale, std::max(this_scale, other_scale)); means that 
I'm not opposed to removing these.  I inserted them mostly to make sure I got 
the proper overloads for the test cases.


Line 176:   DCHECK_EQ(round, false);
> same
Done


PS2, Line 200: shift_width
> ShiftWidth
This is now feeling very much like something that belongs in 
be/src/utils/somewhere


Line 294:       result = scaled_down;
> how about factoring this out into subroutine rather than duplicating?
Done


PS2, Line 328: 127
> should this be 255? (or rather, shift_width()?)
No, r is defined to be int128_t.

However, the round can overflow - we need a bounds check here.


PS2, Line 340: 127
> shift_width()?
Will abstract this out to a rounding helper.  I wrote this before I needed the 
helper.


Line 353:   DCHECK_EQ(round, false);
> same
Done


-- 
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

Reply via email to