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 Wed, 01 Mar 2017 06:09:22 GMT
Zach Amsden has posted comments on this change.

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


Patch Set 8:

(12 comments)

Thanks for the review!

http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.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 
Without the result type change, I have been unable to find a meaningful way to test this.


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)?
Probably.  Our current behavior is more precise.  I like it.


http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/runtime/decimal-test.cc
File be/src/runtime/decimal-test.cc:

Line 635:             result_scale);
> this doesn't look correct when t1.precision - t1.scale + t2.scale + result_
I'll double check - this was supposed to be an exact copy of the logic from the Java code


http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

Line 114:       result += (BitUtil::Sign(v) ^ 1) + 1;
> why is this not just:
That is a better suggestion.


Line 212:       result += (BitUtil::Sign(value) ^ 1) + 1;
> same
Done


PS8, Line 311: sp
> what does "sp" stand for?
single precision (not entirely accurate, but...)

Let me know if you want me to change this


Line 346:         DCHECK(r != 0);
> I don't understand this comment or this dcheck.  Is "precision" referring t
It's actually impossible to get a zero value here (value == non-zero value).  If we have a
remainder (impossible for x == 0), and we didn't need to promote to a higher type, there is
no way to have result (r) != 0 because we should have then promoted to a higher type.

Since the only problematic case is actually getting a zero result, and we can't get that,
we don't need extra bit manipulations for the special case of zero.


Line 347:         r = BitUtil::IncrementAwayFromZero(r);
> given that this turned out to not be generally usable and you have the Sign
I don't mind doing that.


Line 350:     DCHECK(sizeof(RESULT_T) > 8 || abs(r) <= DecimalUtil::MAX_UNSCALED_DECIMAL8);
> what does this DCHECK mean? why is 8 significant?
Either we have to promote to a RESULT_T that can hold the result, or there is no overflow.
 Some of the unit tests violated this condition and required changes (int64_t -> int128_t
for divide)


http://gerrit.cloudera.org:8080/#/c/6132/8/be/src/util/bit-util.h
File be/src/util/bit-util.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 
Sure, but then all the following template conditions will become grotesque:

  typename std::enable_if(std::is_integral<typename std::decay<T>::type>{} ||
  std::is_same<typename std::decay<T>::type{}, unsigned __int128> || ...

Using a template type parameter is better as not only is it more concise, it eliminates many
possibilities for errors in the following by giving everything following the same syntax.

I don't use enable_if lightly, but in this case it is actually prudent to prevent misuse of
the definition on unknown types.


PS8, Line 69: positive
> non-negative
Done


Line 72:   constexpr static inline T IncrementAwayFromZero(T value) {
> but is this worth having anymore given you need to use Sign() in most place
Probably not.  The discovery of the signed zero underflow bias has probably killed the utility
of this utility.


-- 
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: 8
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: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message