impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Date Fri, 27 Oct 2017 21:53:47 GMT
Alex Behm has posted comments on this change. ( )

Change subject: IMPALA-3436: Return a decimal when rounding a double

Patch Set 1:

Commit Message:
PS1, Line 16: This is implemented by introducing a "hack" where we make the parser
How about we say rewrite or translation instead of hack? I don't think it's really a terrible
hack - it's a minimal change.
File be/src/exprs/
PS1, Line 111:   DCHECK(!scale.is_null);
Add a FE test case in that makes sure this is enforced. The existing
round() functions that take a DECIMAL argument must have a non-NULL, constant second argument.
New tests should be added to make sure that is also enforced for your new round() function.

There's also a subtle change in behavior we should consider. The existing round() function
accepts non-constant arguments, should the new round function() also allow that or not?
PS1, Line 116:   bool ovf = false;
File be/src/exprs/decimal-functions.h:
PS1, Line 58:       FunctionContext*, const DoubleVal&, const IntVal&);
add arg names for consistency
File common/function-registry/
PS1, Line 284:   [['round_v1','dround_v1'],
Why keep the dround_v1 alias?
PS1, Line 286:   [['round','dround'], 'DECIMAL', ['DOUBLE', 'INT'], 'impala::DecimalFunctions::RoundTo'],
Can you organize these and comment on whether these are used in v1/v2 or both? Seems confusing
PS1, Line 287:   [['round','dround','round_v1','dround_v1'],
Why keep the dround_v1 alias?
PS1, Line 403:   [['round','dround','round_v1','dround_v1'],
Why keep the dround_v1 alias in these?
File fe/src/main/cup/sql-parser.cup:
PS1, Line 70:   private TQueryOptions queryOptions;
add TODO to remove when decimal v1 is gone, maybe come up with a specific thing you can grep
for later in the code, e.g. DECIMAL_V1 and use that consistently in the TODOs for removal
File fe/src/main/java/org/apache/impala/analysis/
PS1, Line 100:     FunctionCallExpr functionCallExpr = new FunctionCallExpr(fnName, params);
PS1, Line 111:     // nullif(x, y) -> if(x DISTINCT FROM y, x, NULL)
PS1, Line 385:    * This can only be called for functions that return wildcard decimals and
the first
Is this comment still accurate?
PS1, Line 407:       // The situtation where none of the parameters are decimal, but the return
type is
* Shrink comment to:
None of the parameters are decimal but the return type is decimal.

* It's not clear to me why we'd use (38,0) in this case, can you explain? For example, round(double,
int) does not have decimal args. Shouldn't the return type be (38,X) where X is the value
of the second arg if it is a constant?

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
Gerrit-Change-Number: 8398
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Comment-Date: Fri, 27 Oct 2017 21:53:47 +0000
Gerrit-HasComments: Yes

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