impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5019: Decimal V2 addition
Date Thu, 26 Oct 2017 00:18:26 GMT
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8309 )

Change subject: IMPALA-5019: Decimal V2 addition
......................................................................


Patch Set 2:

(12 comments)

I spent some time getting to understand the code. Still don't 100% comprehend it but getting
closer.

http://gerrit.cloudera.org:8080/#/c/8309/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8309/1//COMMIT_MSG@31
PS1, Line 31: to avoid this by first checking if the result would into int128.
fit?


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

http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@166
PS2, Line 166: LeadingZeroAdjustment
I think the name could make it clearer what it's computing. Maybe something like MinLeadingZerosAfterScaling()


http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@170
PS2, Line 170: floor(log2(b))
The table values are actually floor(log2(10^b)) for i = 0, 1, 2, 3... etc, right? I found
this notation a bit confusing.


http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@181
PS2, Line 181: same_sign
We usually make numeric/bool template arguments upper case to make it clear they're constants.

I'm not sure that templates are really necessary here though. It seems like we could just
factor out the shared parts into helper functions and have different functions implementing
the same sign/different sign algorithms. E.g.

  SeparateFractional(x, y, x_scale, y_scale, &x_left, &x_right, &y_left, &y_right);
  int max_scale = std::max(x_scale, y_scale);
  int delta_scale = max_scale - result_scale;
  ... do the same/different sign algorithm ...
 ...


http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@185
PS2, Line 185:   // This is expected to be handled by the caller.
What about the case when they're zero? That's also meant to be handled by the caller, right?
Do we test cases with multiplying by zero?


http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@272
PS2, Line 272:     DCHECK_EQ(result_scale, std::max(this_scale, other_scale));
A brief comment that this is guaranteed by the frontend migh help people new to the code.


http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@277
PS2, Line 277:       DCHECK(!*overflow) << "Cannot overflow unless result is Decimal16Value";
extra indent here


http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@287
PS2, Line 287:   if (this_scale < other_scale) {
It might be slightly easier to follow if the branches were(delta_scale < 0) and (delta_scale
> 0) - it's less obvious this way that it's forcing delta_scale to be positive.


http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@299
PS2, Line 299: into
long line.


http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@305
PS2, Line 305:     bool ovf = AdjustToSameScale(*this, this_scale, other, other_scale,
It feels a bit weird that we're calculating delta_scale above and inside AdjustToSameScale()
- it seems like the delta_scale logic in this function and AdjustToSameScale() are tightly
coupled - AdjustToSameScale() doesn't really abstract it away if the caller has to know the
details to correctly adjust the output result. I'm not sure if there's a better way to factor
it.


http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@310
PS2, Line 310:     if (delta_scale > 0) {
This might need a brief comment to explain why it's necessary (or perhaps there's some way
we can improve how AdjustToSameScale() is factored so that it's clearer what the expected
result scale of that is).


http://gerrit.cloudera.org:8080/#/c/8309/2/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
File fe/src/main/java/org/apache/impala/analysis/TypesUtil.java:

http://gerrit.cloudera.org:8080/#/c/8309/2/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java@228
PS2, Line 228:    * TODO: implement V2 rules for ADD/SUB.
TODO not needed.



-- 
To view, visit http://gerrit.cloudera.org:8080/8309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I401049c56d910eb1546a178c909c923b01239336
Gerrit-Change-Number: 8309
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 00:18:26 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message