You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org> on 2017/10/26 03:02:05 UTC
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8398
Change subject: IMPALA-3436: Return a decimal when rounding a double
......................................................................
IMPALA-3436: Return a decimal when rounding a double
Before this patch, the function round(double, int) returned a double,
which is inconsistent with other round() functions.
In this patch, we change the return type of round(double, int) to be
decimal only if decimal_v2 is enabled. If decimal_v2 is not enabled,
the function returns a double, as before.
This is implemented by introducing a "hack" where we make the parser
aware of query options and rename the function to round_v1() during
parsing if decimal_v2 is not enabled. To make this hack invisible, we
also rename it back to round() in toSql().
This hack should be removed when we remove decimal v1.
Testing:
- Added EE tests.
Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
---
M be/src/exprs/decimal-functions-ir.cc
M be/src/exprs/decimal-functions.h
M be/src/exprs/decimal-operators.h
M common/function-registry/impala_functions.py
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
8 files changed, 227 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/8398/1
--
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: newchange
Gerrit-Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
Gerrit-Change-Number: 8398
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/8398 )
Change subject: IMPALA-3436: Return a decimal when rounding a double
......................................................................
IMPALA-3436: Return a decimal when rounding a double
Before this patch, the function round(double, int) returned a double,
which is inconsistent with other round() functions.
In this patch, we change the return type of round(double, int) to be
decimal only if decimal_v2 is enabled. If decimal_v2 is not enabled,
the function returns a double, as before.
This is implemented by introducing a translateion where we make the
parser aware of query options and rename the function to round_v1()
during parsing if decimal_v2 is not enabled. To make this translation
invisible, we also rename it back to round() in toSql().
This translation should be removed when we remove decimal v1.
Testing:
- Added EE tests.
Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
---
M be/src/exprs/decimal-functions-ir.cc
M be/src/exprs/decimal-functions.h
M be/src/exprs/decimal-operators.h
M common/function-registry/impala_functions.py
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
10 files changed, 264 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/8398/3
--
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: newpatchset
Gerrit-Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
Gerrit-Change-Number: 8398
Gerrit-PatchSet: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> My point about "the alias does not solve the limitations of our V2 function
But is that even possible to implement given that we have to statically determine the return precision and scale for decimal?
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> I agree could be legit uses cases for non-const precisions.
I don't think there's anything inherently wrong with a version of round() that returns the nearest floating point number to the exact answer. That's the nature of all floating point calculations.
It's clearly not the right tool for 99% of queries, but the question should be whether it's ever useful. I think there are some fraction of queries where it probably is the best tool available. Generally I don't think we should remove such things without ensuring that an alternative is available, unless it's really unavoidable. Otherwise we put users in a bind when they want to upgrade because they need to rethink their queries totally instead of just adjusting them.
There is some precedent for this version of round(), e.g. the below systems all return floating point values if given a floating point input:
https://docs.snowflake.net/manuals/sql-reference/functions/round.html
https://docs.microsoft.com/en-us/sql/t-sql/functions/round-transact-sql
http://docs.aws.amazon.com/redshift/latest/dg/r_ROUND.html
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> Just to clarify. I think what Alex is saying is that the proper fix is so t
Right, but is that even possible?
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 16:22:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 3:
I'm clearly being outvoted but I still want to make sure that we've thought it through carefully.
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 16:23:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 3:
(3 comments)
Didn't look at the frontend part.
http://gerrit.cloudera.org:8080/#/c/8398/3/be/src/exprs/decimal-functions-ir.cc
File be/src/exprs/decimal-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8398/3/be/src/exprs/decimal-functions-ir.cc@119
PS3, Line 119: Decimal16Value
Do we always need Decimal16? Or should we switch on ByteSize() like the other functions?
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
I wonder if we should keep the functionality around under another alias in case someone is actually using it with a variable precision. It's an edge case but I can imagine someone having a dimension table that specifies formatting, or having some clever logic to round things to different levels of precision. It would be nice to give them a way out if decimal_v2 breaks them.
We use the "f" prefix in some cases like this, e.g. fmod.
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@407
PS1, Line 407: Type == nu
situation
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Nov 2017 01:29:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky 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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> I agree could be legit uses cases for non-const precisions.
Just to clarify. I think what Alex is saying is that the proper fix is so that the round function looks like this: round(double, int) -> decimal AND the second argument can be non-const. The limitation in this patch is that the second arg must be const. Alex is also saying that there should not exist a round() function that returns a double in decimal v2 (and I agree with this).
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 06:15:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/8398 )
Change subject: IMPALA-3436: Return a decimal when rounding a double
......................................................................
IMPALA-3436: Return a decimal when rounding a double
Before this patch, the function round(double, int) returned a double,
which is inconsistent with other round() functions.
In this patch, we change the return type of round(double, int) to be
decimal only if decimal_v2 is enabled. If decimal_v2 is not enabled,
the function returns a double, as before.
This is implemented by introducing a translateion where we make the
parser aware of query options and rename the function to round_v1()
during parsing if decimal_v2 is not enabled. To make this translation
invisible, we also rename it back to round() in toSql().
This translation should be removed when we remove decimal v1.
Testing:
- Added EE tests.
Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
---
M be/src/exprs/decimal-functions-ir.cc
M be/src/exprs/decimal-functions.h
M be/src/exprs/decimal-operators.h
M common/function-registry/impala_functions.py
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
10 files changed, 264 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/8398/2
--
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: newpatchset
Gerrit-Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
Gerrit-Change-Number: 8398
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Oct 2017 21:53:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky 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 4: Code-Review-1
The current plan is to go in a completely different direction. We want to implement the following rule for round() functions:
The output type of a round() function must match the input type. This patch does the opposite.
I will abandon this patch next week, after thinking about it some more.
--
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: 4
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 18 Nov 2017 00:36:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky 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 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8398/3/be/src/exprs/decimal-functions-ir.cc
File be/src/exprs/decimal-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8398/3/be/src/exprs/decimal-functions-ir.cc@119
PS3, Line 119: Decimal16Value
> Do we always need Decimal16? Or should we switch on ByteSize() like the oth
Yes, the result is a decimal with precision 38, so it always requires 16 bytes.
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> I wonder if we should keep the functionality around under another alias in
Ok, are you suggesting to leave the code as is right now and add "fround" when we get rid of decimal_v1?
Also, added comment.
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@289
PS3, Line 289: [['round','dround','round_v1','dround_v1'],
> This function is used both decimal_v1/v2 modes.
Done
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@407
PS1, Line 407: Type == nu
> situation
This comment was modified in patch 2.
http://gerrit.cloudera.org:8080/#/c/8398/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/8398/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2439
PS3, Line 2439: AnalyzesOk("select round(cast(1.123 as double), int_col) from functional.alltypes",
> add a positive test with a non-NULL constant second argument to both v1/v2
Done
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 00:35:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> We're not keeping decimal_v1 around indefinitely though are we?
* Yes, we want to remove decimal_v1.
* The alias does not solve the limitations of our V2 functions.
* For users that do not want to switch to V2 the V1 workaround is acceptable.
* For users who want to switch to V2 it seems weird to tell them to use a V1-semantics function as workaround, i.e., they'd get inconsistent rounding behavior in their workload or even within the same query.
I think it's better to be clear that it doesn't work in V2.
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 23:25:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> Users typically aren't happy with workarounds that involve changing their q
There are plenty of edge cases though where users that relied on decimal_v1 behaviour would have to modify their queries to use decimal_v2 semantics, so that's unavoidable when we remove decimal_v1.
My concern was that in this case, even if they can modify their queries, then there is no rounding function that can take a variable precision.
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> If users rely on that behavior they can use DECIMAL_V1.
We're not keeping decimal_v1 around indefinitely though are we?
Seems more of a pain to file a JIRA and go through code review again than to just add an alias to the existing floating-point rounding function.
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 23:03:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> If users rely on that behavior they can use DECIMAL_V1.
Users typically aren't happy with workarounds that involve changing their queries.
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 22:45:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> I don't think there's anything inherently wrong with a version of round() t
I still disagree but will commit to adding the alias in the interest of progress.
I don't think adding or maintaining features that are rarely useful makes a lot of sense, especially if the introduce the possibility if inconsistent results.
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 21:28:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has abandoned this change. ( http://gerrit.cloudera.org:8080/8398 )
Change subject: IMPALA-3436: Return a decimal when rounding a double
......................................................................
Abandoned
We decided that rounding a double should return a double (instead of a decimal).
--
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: abandon
Gerrit-Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
Gerrit-Change-Number: 8398
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> * Yes, we want to remove decimal_v1.
To me the point of the JIRA was to change the default behaviour of round(), not to prevent users from rounding to a floating point number with a non-constant argument if that's what they want to do. That seems to be going further than the JIRA really requires.
I don't entirely understand what you mean by "the alias does not solve the limitations of our V2 functions". Do you just mean that the two versions have different pros and cons? If so, then it seems like we should let users make an informed decision about which function they actually want. Or do you mean that we should somehow add support for non-constant arguments to the V2 version?
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> I agree with Alex. I don't think it makes much sense to have a round() func
It's totally plausible to me that someone would write a query with a non-constant precision for round(). E.g. they have a case statement that has some logic to choose precision based on a column value. I wouldn't underestimate the creativity of people writing complex SQL statements.
If we completely remove the ability to do that then there's no workaround once we remove decimal_v1.
If you want to take that risk that's fine, but I don't see a compelling reason not to leave a floating point version accessible as a mitigation.
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 01:48:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky 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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> * Yes, we want to remove decimal_v1.
I agree with Alex. I don't think it makes much sense to have a round() function that returns a double.
We can clearly document that round(double, int) requires a constant second argument in decimal_v2. If it break someone, they were probably doing something weird in the first place.
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 00:26:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 3: Code-Review+1
(4 comments)
I'm happy with the FE changes. Would be good to get a second pair of eyes on the BE portion.
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);
> Added a test.
Agree it's good to be consistent, but it might bite users when we switch DECIMAL_V2 to the default (their queries that have non-constant args in round() will not analyze). We should be sure to clearly document this new behavior/limitation.
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: # TODO: Remove when DECIMAL_V1 is removed.
> Because what if someone typed in "select dround(x)". We need to be able to
Good point. I was thinking dround() vs round() in toSql() doesn't matter that much, but probably it's better to preserve the existing behavior like you suggest.
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@289
PS3, Line 289: [['round','dround','round_v1','dround_v1'],
This function is used both decimal_v1/v2 modes.
http://gerrit.cloudera.org:8080/#/c/8398/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/8398/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2439
PS3, Line 2439: AnalyzesOk("select round(cast(1.123 as double), int_col) from functional.alltypes",
add a positive test with a non-NULL constant second argument to both v1/v2
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Nov 2017 01:26:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> It's totally plausible to me that someone would write a query with a non-co
I agree could be legit uses cases for non-const precisions.
Regarding mitigation: I hope we all agree that a round() returning a double makes no sense. So even if we add a fround() alias we will need a strategy for eventually getting rid of it at a later time. In the meantime, we'll need to preserve the code and tests to make sure fround() does not break, even if we remove decimal_v1. So to me it feels like fround() is prolonging the inevitable removal of round() returning a double.
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 05:00:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8398/3/be/src/exprs/decimal-functions-ir.cc
File be/src/exprs/decimal-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8398/3/be/src/exprs/decimal-functions-ir.cc@119
PS3, Line 119: Decimal16Value
> Yes, the result is a decimal with precision 38, so it always requires 16 by
Got it
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: # Althernatively, consider renaming "round_v1" to "fround".
> Ok, are you suggesting to leave the code as is right now and add "fround" w
Might as well add it now if we're going to add it.
--
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: 4
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 22:00:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/8398 )
Change subject: IMPALA-3436: Return a decimal when rounding a double
......................................................................
IMPALA-3436: Return a decimal when rounding a double
Before this patch, the function round(double, int) returned a double,
which is inconsistent with other round() functions.
In this patch, we change the return type of round(double, int) to be
decimal only if decimal_v2 is enabled. If decimal_v2 is not enabled,
the function returns a double, as before.
This is implemented by introducing a translateion where we make the
parser aware of query options and rename the function to round_v1()
during parsing if decimal_v2 is not enabled. To make this translation
invisible, we also rename it back to round() in toSql().
This translation should be removed when we remove decimal v1.
Testing:
- Added EE tests.
Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
---
M be/src/exprs/decimal-functions-ir.cc
M be/src/exprs/decimal-functions.h
M be/src/exprs/decimal-operators.h
M common/function-registry/impala_functions.py
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
10 files changed, 268 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/8398/4
--
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: newpatchset
Gerrit-Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
Gerrit-Change-Number: 8398
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> Might as well add it now if we're going to add it.
If users rely on that behavior they can use DECIMAL_V1.
Allowing non-constant 2nd args in DECIMAL_V2 is really a new feature. If we want to that then imo we should focus on fixing the current functions to allow that in a separate patch.
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 22:41:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285: [['round_v1','dround_v1'],
> To me the point of the JIRA was to change the default behaviour of round(),
My point about "the alias does not solve the limitations of our V2 functions" means that the V2 round() will still not accept non-constant 2nd args. A sensible user that wants to use non-const args would probably want consistent semantics, so imo the real fix is to allow non-const args in the V2 round().
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 04:55:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht 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 3:
Taras, can you summarize where we are at (should reviews continue?) Or even -1 it if the plan has changed (or isn't yet known).
--
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: 3
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Nov 2017 18:05:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky 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
Done
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 enforce
Added a test.
I think we should remain consistent with other decimal round functions. They do not allow a non-constant round arguments, so I think it makes sense to not allow it here as well.
http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions-ir.cc@116
PS1, Line 116: bool ovf = false;
> overflow
Done
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
Done
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?
Because what if someone typed in "select dround(x)". We need to be able to differentiate between round and dround in error messages.
Are you suggesting to get rid of the "dround" alias completely?
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 bo
Done
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?
Same reason as above.
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?
Answered above.
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 t
Done
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
Done
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
Done
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?
Fixed the comment.
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:
Done
--
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 <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Nov 2017 00:35:00 +0000
Gerrit-HasComments: Yes