impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
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. ( )

Change subject: IMPALA-5019: Decimal V2 addition

Patch Set 2:


I spent some time getting to understand the code. Still don't 100% comprehend it but getting
Commit Message:
PS1, Line 31: to avoid this by first checking if the result would into int128.
File be/src/runtime/decimal-value.inline.h:
PS2, Line 166: LeadingZeroAdjustment
I think the name could make it clearer what it's computing. Maybe something like MinLeadingZerosAfterScaling()
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.
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 ...
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?
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.
PS2, Line 277:       DCHECK(!*overflow) << "Cannot overflow unless result is Decimal16Value";
extra indent here
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.
PS2, Line 299: into
long line.
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
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).
File fe/src/main/java/org/apache/impala/analysis/
PS2, Line 228:    * TODO: implement V2 rules for ADD/SUB.
TODO not needed.

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zach Amsden <>
Gerrit-Comment-Date: Thu, 26 Oct 2017 00:18:26 +0000
Gerrit-HasComments: Yes

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