impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
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. ( http://gerrit.cloudera.org:8080/8398 )

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


Patch Set 1:

(13 comments)

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

http://gerrit.cloudera.org:8080/#/c/8398/1//COMMIT_MSG@16
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.


http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions-ir.cc
File be/src/exprs/decimal-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions-ir.cc@111
PS1, Line 111:   DCHECK(!scale.is_null);
Add a FE test case in AnalyzeExprsTest.java 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?


http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions-ir.cc@116
PS1, Line 116:   bool ovf = false;
overflow


http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions.h
File be/src/exprs/decimal-functions.h:

http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions.h@58
PS1, Line 58:       FunctionContext*, const DoubleVal&, const IntVal&);
add arg names for consistency


http://gerrit.cloudera.org:8080/#/c/8398/1/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:

http://gerrit.cloudera.org:8080/#/c/8398/1/common/function-registry/impala_functions.py@284
PS1, Line 284:   [['round_v1','dround_v1'],
Why keep the dround_v1 alias?


http://gerrit.cloudera.org:8080/#/c/8398/1/common/function-registry/impala_functions.py@286
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
now.


http://gerrit.cloudera.org:8080/#/c/8398/1/common/function-registry/impala_functions.py@287
PS1, Line 287:   [['round','dround','round_v1','dround_v1'],
Why keep the dround_v1 alias?


http://gerrit.cloudera.org:8080/#/c/8398/1/common/function-registry/impala_functions.py@403
PS1, Line 403:   [['round','dround','round_v1','dround_v1'],
Why keep the dround_v1 alias in these?


http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/cup/sql-parser.cup@70
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


http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@100
PS1, Line 100:     FunctionCallExpr functionCallExpr = new FunctionCallExpr(fnName, params);
newline


http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@111
PS1, Line 111:     // nullif(x, y) -> if(x DISTINCT FROM y, x, NULL)
newline


http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@385
PS1, Line 385:    * This can only be called for functions that return wildcard decimals and
the first
Is this comment still accurate?


http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@407
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 http://gerrit.cloudera.org:8080/8398
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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 <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Oct 2017 21:53:47 +0000
Gerrit-HasComments: Yes

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