Dan Hecht has posted comments on this change.
Change subject: IMPALA4813: Round on divide and multiply
......................................................................
Patch Set 8:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/exprs/exprtest.cc
File be/src/exprs/exprtest.cc:
Line 1339: { "1.23 * cast(1 as decimal(20,3))", {{ false, 123000, 23, 5 }}},
i thought you said there were some multiplies you could do to exercise the multiply rounding
path?
Line 1408: // N.B.  Google and python both insist that 999999.998 / 999 is 1001.000999
doesn't that just mean they use a scale at 6 (and round)?
http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/runtime/decimaltest.cc
File be/src/runtime/decimaltest.cc:
Line 635: result_scale);
this doesn't look correct when t1.precision  t1.scale + t2.scale + result_scale > 38.
In that case, we reduce the scale (but always preserve at least 6).
http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/runtime/decimalvalue.inline.h
File be/src/runtime/decimalvalue.inline.h:
Line 114: result += (BitUtil::Sign(v) ^ 1) + 1;
why is this not just:
result += BitUtil::Sign(v);
?
Line 212: result += (BitUtil::Sign(value) ^ 1) + 1;
same
PS8, Line 311: sp
what does "sp" stand for?
Line 346: DCHECK(r != 0);
I don't understand this comment or this dcheck. Is "precision" referring to decimal precision?
And precision of what? and what do you mean by "always have a value"? nonnegative value?
and why is this case fundamentally different than the sizeof(T) == 16 case?
it doesn't seem true that rounding is needed if r != 0, but if r == 0, then rounding is never
needed...
Line 347: r = BitUtil::IncrementAwayFromZero(r);
given that this turned out to not be generally usable and you have the Sign() abstraction,
how about getting rid of this and just using:
r += Sign(r);
here?
Line 350: DCHECK(sizeof(RESULT_T) > 8  abs(r) <= DecimalUtil::MAX_UNSCALED_DECIMAL8);
what does this DCHECK mean? why is 8 significant?
http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/util/bitutil.h
File be/src/util/bitutil.h:
Line 52: std::is_same<CVR_REMOVED, __int128>{}, int>::type
= 0>
can CVR_REMOVED be defined local to UnsignedWidth(), or does that not work for some reason?
PS8, Line 69: positive
nonnegative
Line 72: constexpr static inline T IncrementAwayFromZero(T value) {
but is this worth having anymore given you need to use Sign() in most places, and that using
this is the wrong thing to do when there are fractional parts?

To view, visit http://gerrit.cloudera.org:8080/6132
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
GerritMessageType: comment
GerritChangeId: Ie6bfcbe37555b74598d409c6f84f06b0ae5c4312
GerritPatchSet: 8
GerritProject: ImpalaASF
GerritBranch: master
GerritOwner: Zach Amsden <zamsden@cloudera.com>
GerritReviewer: Dan Hecht <dhecht@cloudera.com>
GerritReviewer: Michael Ho
GerritReviewer: Michael Ho <kwho@cloudera.com>
GerritReviewer: Zach Amsden <zamsden@cloudera.com>
GerritHasComments: Yes
