impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
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:

(12 comments)

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


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?


PS2, Line 155: /* round */ true
why?


PS2, Line 169: /* round */ true
same


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 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);
same


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


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

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
shift_width()?


Line 353:   DCHECK_EQ(round, false);
same


-- 
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 <zamsden@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-HasComments: Yes

Mime
View raw message