impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4813: Round on divide and multiply
Date Fri, 24 Feb 2017 17:27:58 GMT
Dan Hecht has posted comments on this change.

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

Patch Set 2:

File be/src/exprs/

PS2, Line 682: ROUND
I don't see why we need this. it happens that our Add() et al result types don't need rounding,
but that's because of how we choose their result type.  i.e. these operations already handle
rounding enabled and disabled correctly (since the answer is the same, and they have the dchceks
in place already for not losing scale). 

But that's a detail that the decimal-value code cares about, not this code.  also see my comments
File be/src/runtime/decimal-value.h:

Line 124:       int result_scale, bool* overflow) const {
where are these variants used?

PS2, Line 155: /* round */ true

PS2, Line 169: /* round */ true
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 we never lose scale
and so rounding is a no-op, so why not allow the caller to pass the actual value of 'round'?
 One day, we may change the result type of Add(), and in some cases lose scale, in which case
round will no longer be a no-op and at that point the dcheck at line 152 would no longer hold
telling us that this code needs to be updated.

Line 176:   DCHECK_EQ(round, false);

Line 198: // Helper because int128_t is not part of std::numeric_limits
explain what this does

PS2, Line 200: shift_width
though the name doesn't really tell you what it does. maybe the subroutine should be something

T IncrementAwayZero(T value) {
   int shift_by = ... stuff here...
   return value + (1 | value >> shift_by);

and then you can use that all over.

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

PS2, Line 328: 127
should this be 255? (or rather, shift_width()?)

PS2, Line 340: 127

Line 353:   DCHECK_EQ(round, false);

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6bfcbe37555b74598d409c6f84f06b0ae5c4312
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Michael Ho
Gerrit-HasComments: Yes

View raw message