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
