You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Philip Zeyliger (Code Review)" <ge...@cloudera.org> on 2017/08/25 18:45:05 UTC
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Philip Zeyliger has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7829
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
This commit simplifies the following conditional functions:
* istrue
* isfalse
* isnottrue
* isnotfalse
* zeroifnull
* nullifzero
* nullvalue
* nonnullvalue
* nullif
Most of these simplifications work when the arguments to
these functions are literals. The one exception is
that nullif(x, y) simplifies to NULL when x.equals(y)
as expressions. This is valid since nullif(null, null)=null.
This simplification has not been applied to the equivalent
"case x=x then null else null".
In terms of the implementation is a bit of a toss-up between re-writing
these into their equivalent case statements or simplifying them here.
Testing:
* Added new tests to ExprRewriteRulesTest
* Confirmed (using EclEmma, which uses jococo engine) that coverage is
good.
Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
2 files changed, 200 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/1
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5211: Simplifying ifnull conditional.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5211: Simplifying ifnull conditional.
......................................................................
Patch Set 5:
* Correct! The BE code and all the associated function plumbing for IFNULL should be removed.
* We should do the transformation even before analysis, like we do for NVL2 (see FunctionCallExpr#createExpr). I don't think we can get into a weird situation with NullLiterals.
I like your idea of handling "[not] distinct from" in a separate rule.
I agree that the docs are wrong, I'll let you decide whether to file a JIRA or fix it yourself.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
Patch Set 7:
I should also mention that previous commit messages referred to "ifnull" where they should have referred to "nullif". It's not surprising that these are confusing, but it's worth noting that we didn't catch that in several rounds of reading.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
IMPALA-5211: Simplifying nullif conditional.
This commit:
* Converts nullif(x, y) into if(x IS DISTINCT FROM y, x, NULL).
* Re-writes x IS DINSTINCT FROM y -> FALSE if x.equals(y).
* Removes backend implementation of nullif.
As is the case with all conversions, the original nullif(...) is
replaced with if(...) in error messages, explain plans, and so on.
It's important and subtle that the conversion uses "x IS DISTINCT FROM y"
rather than "x != y" so that the simplification can be made while
handling null values correctly. ("x != x" may be either false or null,
but x is distinct from x is always false.)
Testing:
* Added new tests to ExprRewriteRulesTests for nullif and the if(x
distinct from y, ...) simplification.
* New test for the rewrite in ParserTest.
* Adds an nvl2() test, incidentally.
* Confirmed (using EclEmma, which uses jococo engine) that coverage is good.
* Ran the tests.
Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Reviewed-on: http://gerrit.cloudera.org:8080/7829
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
A fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
19 files changed, 205 insertions(+), 122 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Alex Behm: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 48: class CountingRewriteRuleWrapper implements ExprRewriteRule {
> I was thinking along the lines that nobody outside of ExprRewriteRulesTest
Done
http://gerrit.cloudera.org:8080/#/c/7829/8/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 606: RewritesOk("if(true, nullif(tinyint_col, int_col), null)",
> I don't understand what coverage this adds. The ParserTest already checks t
Ok, removed.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
Patch Set 2:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/7829/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
PS2, Line 145: /**
: * Helper for simplifying unary boolean function (like istrue(x)).
: */
: private Expr unaryHelper(Expr expr, LiteralExpr child,
: boolean onTrue, boolean onFalse, boolean onNull)
I'm not crazy about this interface because I had to read all the code to really understand what it was doing. I don't have a better suggestion yet.
PS2, Line 153: Boolean
is there a reason you use Boolean over boolean? Given getValue() returns the primitive type, I'd prefer the primitive type so that readers don't have to reason about this being null.
PS2, Line 154: val
also, I don't see a reason not to getValue() inline here
PS2, Line 165:
: /**
: * Helper for simplifying unary functions like isnull(x).
: */
: private Expr unaryHelper2(Expr expr, LiteralExpr child, boolean onNull,
: boolean onOther)
same
Line 179: private Expr simplifyFunctionCallExpr(FunctionCallExpr expr, Analyzer analyzer) throws AnalysisException {
nit: long line, please wrap at 90
PS2, Line 197: // ontrue onfalse onnull
: if (fnName.equals("isfalse")) return unaryHelper(expr, c, false, true, false);
nit: we typically don't use cool whitespace formatting to line things up. I don't feel strongly though, and in this case it does seem to be helpful. That said, I think we should try to find a way to make the function names/parameters a bit easier to understand on their own.
PS2, Line 235: x
y
Line 237: * Note that we currently don't simplify all possible equal expressions
This may be obvious, but not to me. Would you mind elaborating? I understand why the cast case can't be simplified, but not the case involving argument ordering.
Line 243: private Expr simplifyNullIfFunctionCallExpr(Expr expr, Analyzer analyzer) throws AnalysisException {
nit long line
PS2, Line 248: literalExprsEqual
can you explain why this does constant folding but other optimizations do not?
PS2, Line 300: Rewrites a = b
Doesn't indicate that this creates Expr 'a = b'. How about:
Returns a new Expr 'a = b', after folding constants.
PS2, Line 303: literal
this fn doesn't appear to require literal exprs. A more precise name might be something like: foldedExprsEqualExpr
PS2, Line 306: Expr rewritten = analyzer.getConstantFolder().rewrite(pred, analyzer);
: return rewritten;
remove local var and return result
PS2, Line 315: return rewritten instanceof BoolLiteral &&
: ((BoolLiteral) rewritten).getValue();
nit 1line? It looks less than 90chars...
http://gerrit.cloudera.org:8080/#/c/7829/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
PS2, Line 424: // Exhaustive test for istrue and friends on boolean literals.
It'd be helpful to have a few more cases where you have constant expressions, e.g. including some cases that don't get rewritten (istrue(true and true))
PS2, Line 426: // $for f in istrue isnottrue isfalse isnotfalse; do
: // for y in true false null; do
: // echo 'RewritesOk("'"$f($y)"'", rule, "'$(impala-shell.sh -B --quiet --query "select $f($y)" 2> /dev/null | tr '[a-z]' '[A-Z]')'");'
: // done
: // done
fancy
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7829/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
Line 220: * Simplifies nullif(a, b) when a and b are identical
> Sorry I got confused by the nullif() semantics, my suggestion with special
Argh, wrong suggestion again, it should be:
NULLIF(x, y) -> IF(x <=> y, NULL, x)
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has uploaded a new patch set (#2).
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
This commit simplifies the following conditional functions:
* istrue
* isfalse
* isnottrue
* isnotfalse
* zeroifnull
* nullifzero
* nullvalue
* nonnullvalue
* nullif
Most of these simplifications work when the arguments to
these functions are literals. The one exception is
that nullif(x, y) simplifies to NULL when x.equals(y)
as expressions. This is valid since nullif(null, null)=null.
This simplification has not been applied to the equivalent
"case x=x then null else null".
In terms of the implementation is a bit of a toss-up between re-writing
these into their equivalent case statements or simplifying them here.
Testing:
* Added new tests to ExprRewriteRulesTest
* Confirmed (using EclEmma, which uses jococo engine) that coverage is
good.
Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
2 files changed, 200 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/2
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
Patch Set 7:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
Line 49: * x is [not] distinct from x -> false [true]
> this is handled in your new rule
Done
http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
Line 1642: AnalysisError("select nullif(1,2,3)", "default.nullif() unknown");
> Weird. We should consider fixing this later by perhaps adding a dummy nulli
Filed https://issues.apache.org/jira/browse/IMPALA-5938.
http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 48: static class CountingRewriteRuleWrapper implements ExprRewriteRule {
> static not needed
I took it out.
I put it in because this prevents the inner class from referencing ExprRewriteRulesTest.this. Is your consideration here that we should just be as sparse as possible for test code?
Line 58: if (expr != ret) {
> single line
Done
Line 128: // All specified rules must have fired at least once.
> Explain why.
Done
Line 312: RewritesOk("nullif(NULL, NULL)", rule, "NULL");
> Did you check whether these new tests are not already addressed by expr-tes
nullif, decode, and nvl2 all have some coverage in expr-test. I've removed this.
Line 579: public void TestDistinctFromSimplificationRule() throws AnalysisException {
> TestSimplifyDistinctFromRule()
Done
Line 603: * it can be further simplified via DistinctFromSimplificatinoRule.
> SimplifyDistinctFromRule
Done
Line 619: // highlights that nullif gets transformed; the test infrastructure
> I don't get the part about the test infrastructure.
updated comment.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
Patch Set 2:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/7829/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
PS2, Line 145: /**
: * Helper for simplifying unary boolean function (like istrue(x)).
: */
: private Expr unaryHelper(Expr expr, LiteralExpr child,
: boolean onTrue, boolean onFalse, boolean onNull)
> I'm not crazy about this interface because I had to read all the code to re
I added a few more comments here. I don't have a better name idea. "isBooleanHelper()" seems no better.
I moved this below the code that uses it so it reads easier for reviewers.
PS2, Line 153: Boolean
> is there a reason you use Boolean over boolean? Given getValue() returns th
Should have been boolean; fixed by inlining.
PS2, Line 154: val
> also, I don't see a reason not to getValue() inline here
Done
PS2, Line 165:
: /**
: * Helper for simplifying unary functions like isnull(x).
: */
: private Expr unaryHelper2(Expr expr, LiteralExpr child, boolean onNull,
: boolean onOther)
> same
I got rid of this method entirely.
Line 179: private Expr simplifyFunctionCallExpr(FunctionCallExpr expr, Analyzer analyzer) throws AnalysisException {
> nit: long line, please wrap at 90
Done
PS2, Line 197: // ontrue onfalse onnull
: if (fnName.equals("isfalse")) return unaryHelper(expr, c, false, true, false);
> nit: we typically don't use cool whitespace formatting to line things up. I
I debated using whitespace here vs. doing the easy thing, and I decided that it was a little bit easier to read when the truth table is right here in front of you. I'm happy to be overridden and just to have Eclipse re-format this, but I do think it's easier to read this way.
I also thought about doing crazy bit math and nested ternary operators. I didn't go that route, as I figured I couldn't read it.
The implementation for the C++ half of these is
in be/src/exprs/conditional-functions-ir.cc. I looked and I could obviously write four functions to parallel that implementation, but it seems no better.
PS2, Line 235: x
> y
Done
Line 237: * Note that we currently don't simplify all possible equal expressions
> This may be obvious, but not to me. Would you mind elaborating? I understan
Updated the comment.
Line 243: private Expr simplifyNullIfFunctionCallExpr(Expr expr, Analyzer analyzer) throws AnalysisException {
> nit long line
Done
PS2, Line 248: literalExprsEqual
> can you explain why this does constant folding but other optimizations do n
Done
PS2, Line 300: Rewrites a = b
> Doesn't indicate that this creates Expr 'a = b'. How about:
Done
PS2, Line 303: literal
> this fn doesn't appear to require literal exprs. A more precise name might
Done
PS2, Line 306: Expr rewritten = analyzer.getConstantFolder().rewrite(pred, analyzer);
: return rewritten;
> remove local var and return result
Sure. I think I default to being more verbose because I like to set breakpoints, which leads me to having one thing per line.
PS2, Line 315: return rewritten instanceof BoolLiteral &&
: ((BoolLiteral) rewritten).getValue();
> nit 1line? It looks less than 90chars...
Done
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
Patch Set 10: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5211: Simplifying ifnull conditional.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5211: Simplifying ifnull conditional.
......................................................................
Patch Set 6:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/7829/6//COMMIT_MSG
Commit Message:
Line 9: Re-writes ifnull(x, y) into if(x IS DISTINCT FROM y, x, NULL),
Comment somewhat hard to read, maybe create bullet points foe each change, should be clear which one is a "conversion" and which one adds a new rewrite rule
I prefer "convert" instead of re-write because the latter has
a very specific meaning to us (done via ExprRewriteRule)
Line 18: The error messages are slightly altered by this change:
Not just error messages, might be easier to just state that the original ifnull() is printed as the converted if() everywhere including error messages, explain plans, column labels, etc.
Line 20: Before:
Not sure we need to flesh this out so much. Like you said below, this is what you already get with similar conversions, e.g., NVL2() or decode()
http://gerrit.cloudera.org:8080/#/c/7829/6/be/src/exprs/conditional-functions-ir.cc
File be/src/exprs/conditional-functions-ir.cc:
Line 46
I like red!
http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
Line 85: public static Expr createExpr(FunctionName fnName, FunctionParams params)
I think we should add tests along the lines of what we have for NVL2() in AnalyzeExprsTest#TestFunctionCallExpr
Line 86: throws AnalysisException {
This isn't right - createExpr() is and should only be called from the parser from which we should not throw an AnalysisException. We should strive to keep parsing and analysis distinct phases.
I think the right way to fix the dilemma below is to only transform the NULLIF() into IF() if there are exactly two arguments - otherwise we can just create a FunctionCallExpr with the original NULLIF function name which will later fail gracefully during analysis.
Line 108: NullLiteral.create(Type.NULL) // NULL
Use new NullLiteral(). The create() returns an *analyzed* NullLiteral which is not what you want because we have not started analysis yet (still in parsing).
create() is typically used for creating an analyzed NullLiteral of a specific (non-NULL) type
http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
Line 152: // Unary functions like isfalse, isnotfalse, istrue, isnottrue, nullvalue,
Move to class comment
Line 155: // nullif and nvl2 are re-written into an if.
re-written -> converted
Line 204: * Note that FunctionalCallExpr.createExpr() re-writes "nvl2" into "if",
re-writes -> converts
http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java:
Line 39: public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException {
Doesn't throw. I just noticed we declare throws unnecessarily in a bunch of other rules. Mind cleaning those up, too?
http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 387: RewritesOk("nvl2(null, 1, 2)", rules, "2");
Let's try not to mix these tests. Constant folding should be tested in the constant folding test. The fact that this is a conditional function is secondary. The tests are grouped by rewrite rule - not by type of expr.
Line 424: // nullif: handled by folding
move to constant folding test
Line 583: // nullif: handled by rewrite to if and simplification
converted to if and simplified
Line 584: RewritesOk("nullif(bool_col, bool_col)", rules, "NULL");
Move into SimplifyConditionalsTest
http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:
Line 90: public void ParserAnalysisError(String stmt, String expectedErrorString) {
Parser does analyze.
Also I don't see this used.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying ifnull conditional.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7829
to look at the new patch set (#5).
Change subject: IMPALA-5211: Simplifying ifnull conditional.
......................................................................
IMPALA-5211: Simplifying ifnull conditional.
Re-writes ifnull(x, y) into if(x DISTINCT FROM y, x, NULL),
and simplifies the latter if x.equals(y).
We use "DISTINCT FROM" rather than "=" to handle gracefully
that, if x and y are null, we still want NULL to get returned.
Testing:
* Added new tests to ExprRewriteRulesTests for nullif and the if(x
distinct from y, ...) simplification.
* Adds an nvl2() test, incidentally.
* Confirmed (using EclEmma, which uses jococo engine) that coverage is good.
Questions:
* I think this means we can remove the NullIfExpr and friends
from the backend, since the FE has moved them away. I've not
done that yet.
* The re-write in FunctionCallExpr creates a NullLiteral of type
NULL. The types of the children aren't available yet. I haven't
yet figured out if that's the right thing to do.
Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
3 files changed, 88 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/5
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7829
to look at the new patch set (#9).
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
IMPALA-5211: Simplifying nullif conditional.
This commit:
* Converts nullif(x, y) into if(x IS DISTINCT FROM y, x, NULL).
* Re-writes x IS DINSTINCT FROM y -> FALSE if x.equals(y).
* Removes backend implementation of nullif.
As is the case with all conversions, the original nullif(...) is
replaced with if(...) in error messages, explain plans, and so on.
It's important and subtle that the conversion uses "x IS DISTINCT FROM y"
rather than "x != y" so that the simplification can be made while
handling null values correctly. ("x != x" may be either false or null,
but x is distinct from x is always false.)
Testing:
* Added new tests to ExprRewriteRulesTests for nullif and the if(x
distinct from y, ...) simplification.
* New test for the rewrite in ParserTest.
* Adds an nvl2() test, incidentally.
* Confirmed (using EclEmma, which uses jococo engine) that coverage is good.
* Ran the tests.
Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
---
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
A fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
19 files changed, 205 insertions(+), 122 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/9
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5211: Simplifying ifnull conditional.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7829
to look at the new patch set (#6).
Change subject: IMPALA-5211: Simplifying ifnull conditional.
......................................................................
IMPALA-5211: Simplifying ifnull conditional.
Re-writes ifnull(x, y) into if(x IS DISTINCT FROM y, x, NULL),
and simplifies x IS DISTINCT FROM y -> FALSE if x.equals(y). Removes
backend implementation of ifnull.
It's important and subtle that the re-write uses "x IS DISTINCT FROM y"
rather than "x != y" so that the simplification can be made while
handling null values correctly. ("x != x" may be either false or null,
but x is distinct from x is always false.)
The error messages are slightly altered by this change:
Before:
Query: select nullif("a", 1)
ERROR: AnalysisException: No matching function with signature: nullif(STRING, TINYINT).
Query: select nullif(1, 2, 3)
ERROR: AnalysisException: No matching function with signature: nullif(TINYINT, TINYINT, TINYINT).
After:
ERROR: AnalysisException: operands of type STRING and TINYINT are not comparable: 'a' IS DISTINCT FROM 1
ERROR: AnalysisException: nullif() must be called with two arguments.
This kind of error message is consistent with what you get for other re-writes.
Testing:
* Added new tests to ExprRewriteRulesTests for nullif and the if(x
distinct from y, ...) simplification.
* New test for the rewrite in ParserTest.
* Adds an nvl2() test, incidentally.
* Confirmed (using EclEmma, which uses jococo engine) that coverage is good.
* Ran the tests.
Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
---
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
A fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
12 files changed, 195 insertions(+), 93 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/6
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 48: class CountingRewriteRuleWrapper implements ExprRewriteRule {
> I took it out.
I was thinking along the lines that nobody outside of ExprRewriteRulesTest needs to create an instance of this, but your argument for static makes sense, too. private static then?
http://gerrit.cloudera.org:8080/#/c/7829/8/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 606: RewritesOk("if(true, nullif(tinyint_col, int_col), null)",
I don't understand what coverage this adds. The ParserTest already checks the nullif() to if() conversion. It's confusing to me to test conversion in ExprRewriteRulesTest because the conversion does not happen through a rewrite rule. Also you need to wrap the nullif() in this test with an if() to workaround the new counting check you added... Quite hard to comprehend what's going on here. Please remove unless you insist this adds coverage.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has uploaded a new patch set (#3).
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
This commit simplifies the following conditional functions:
* istrue
* isfalse
* isnottrue
* isnotfalse
* zeroifnull
* nullifzero
* nullvalue
* nonnullvalue
* nullif
Most of these simplifications work when the arguments to
these functions are literals. The one exception is
that nullif(x, y) simplifies to NULL when x.equals(y)
as expressions. This is valid since nullif(null, null)=null.
This simplification has not been applied to the equivalent
"case x=x then null else null".
In terms of the implementation is a bit of a toss-up between re-writing
these into their equivalent case statements or simplifying them here.
Testing:
* Added new tests to ExprRewriteRulesTest
* Confirmed (using EclEmma, which uses jococo engine) that coverage is
good.
Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
2 files changed, 208 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/3
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7829
to look at the new patch set (#7).
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
IMPALA-5211: Simplifying nullif conditional.
This commit:
* Converts nullif(x, y) into if(x IS DISTINCT FROM y, x, NULL).
* Re-writes x IS DINSTINCT FROM y -> FALSE if x.equals(y).
* Removes backend implementation of nullif.
As is the case with all conversions, the original nullif(...) is
replaced with if(...) in error messages, explain plans, and so on.
It's important and subtle that the conversion uses "x IS DISTINCT FROM y"
rather than "x != y" so that the simplification can be made while
handling null values correctly. ("x != x" may be either false or null,
but x is distinct from x is always false.)
Testing:
* Added new tests to ExprRewriteRulesTests for nullif and the if(x
distinct from y, ...) simplification.
* New test for the rewrite in ParserTest.
* Adds an nvl2() test, incidentally.
* Confirmed (using EclEmma, which uses jococo engine) that coverage is good.
* Ran the tests.
Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
---
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
A fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
19 files changed, 230 insertions(+), 122 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/7
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
Patch Set 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1226/
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7829/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
Line 160: if (expr.getChildren().size() == 1) {
Sorry, but I think these simplifications are not needed because they are already addressed by FoldConstantsRule.
Here, we only need to handle conditionals that can be simplified even when non-constant (constant exprs are handled in FoldConstantsRule).
Line 220: * Simplifies nullif(a, b) when a and b are identical
Why is equals() not sufficient on its own? Two identical literals should also return true for equals().
Seems much easier to deal with NullLiteral case specially in this rule, and otherwise only rely on equals().
Alternatively, we could follow the same approach we use for NVL2() - directly convert NULLIF() into an IF() in FunctionCallExpr.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7829
to look at the new patch set (#4).
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
This commit simplifies the following conditional functions:
* istrue
* isfalse
* isnottrue
* isnotfalse
* zeroifnull
* nullifzero
* nullvalue
* nonnullvalue
* nullif
Most of these simplifications work when the arguments to
these functions are literals. The one exception is
that nullif(x, y) simplifies to NULL when x.equals(y)
as expressions. This is valid since nullif(null, null)=null.
This simplification has not been applied to the equivalent
"case x=x then null else null".
In terms of the implementation is a bit of a toss-up between re-writing
these into their equivalent case statements or simplifying them here.
Testing:
* Added new tests to ExprRewriteRulesTest
* Confirmed (using EclEmma, which uses jococo engine) that coverage is
good.
Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
2 files changed, 208 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/4
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7829
to look at the new patch set (#8).
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
IMPALA-5211: Simplifying nullif conditional.
This commit:
* Converts nullif(x, y) into if(x IS DISTINCT FROM y, x, NULL).
* Re-writes x IS DINSTINCT FROM y -> FALSE if x.equals(y).
* Removes backend implementation of nullif.
As is the case with all conversions, the original nullif(...) is
replaced with if(...) in error messages, explain plans, and so on.
It's important and subtle that the conversion uses "x IS DISTINCT FROM y"
rather than "x != y" so that the simplification can be made while
handling null values correctly. ("x != x" may be either false or null,
but x is distinct from x is always false.)
Testing:
* Added new tests to ExprRewriteRulesTests for nullif and the if(x
distinct from y, ...) simplification.
* New test for the rewrite in ParserTest.
* Adds an nvl2() test, incidentally.
* Confirmed (using EclEmma, which uses jococo engine) that coverage is good.
* Ran the tests.
Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
---
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
A fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
19 files changed, 213 insertions(+), 122 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/8
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
Patch Set 6:
(17 comments)
Thanks for the review.
I've addressed all of the comments except moving around TestIfNull: it fundamentally relies on multiple rules to work, so I don't like moving it into one rule or another, but I'm happy to be over-written.
I ended up down a small rabbit hole asserting that all the rules passed into RewriteOk() are really used. This led me to small fixes in the tests. I think it's not intrusive (and found some issues and would have helped me), but if you think otherwise, I can get rid of it.
http://gerrit.cloudera.org:8080/#/c/7829/6//COMMIT_MSG
Commit Message:
Line 9: Re-writes ifnull(x, y) into if(x IS DISTINCT FROM y, x, NULL),
> Comment somewhat hard to read, maybe create bullet points foe each change,
Done
Line 18: The error messages are slightly altered by this change:
> Not just error messages, might be easier to just state that the original if
Done
Line 20: Before:
> Not sure we need to flesh this out so much. Like you said below, this is wh
Ok. Done (removed).
http://gerrit.cloudera.org:8080/#/c/7829/6/be/src/exprs/conditional-functions-ir.cc
File be/src/exprs/conditional-functions-ir.cc:
Line 46
> I like red!
Done
http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
Line 85: public static Expr createExpr(FunctionName fnName, FunctionParams params)
> I think we should add tests along the lines of what we have for NVL2() in A
Done
Line 86: throws AnalysisException {
> This isn't right - createExpr() is and should only be called from the parse
Good idea; done.
Line 108: NullLiteral.create(Type.NULL) // NULL
> Use new NullLiteral(). The create() returns an *analyzed* NullLiteral which
Done
http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
Line 152: // Unary functions like isfalse, isnotfalse, istrue, isnottrue, nullvalue,
> Move to class comment
Done
Line 155: // nullif and nvl2 are re-written into an if.
> re-written -> converted
Done
Line 204: * Note that FunctionalCallExpr.createExpr() re-writes "nvl2" into "if",
> re-writes -> converts
Done
http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java:
Line 39: public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException {
> Doesn't throw. I just noticed we declare throws unnecessarily in a bunch of
I think I cleaned these up. FoldConstantsRule is the only one that still throws anything, and SimplifyConditionalsRule uses a constant folder, so it permeates.
http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 387: RewritesOk("nvl2(null, 1, 2)", rules, "2");
> Let's try not to mix these tests. Constant folding should be tested in the
Ok.
I had to move some decode tests around too, to satisfy this.
And then I went down the small rabbit hole of actually making sure that all the rules specified are applied at least once, which led me to wrap the ExprRewriteRule. With the wrapper, I'm reasonably convinced that all the tests here exercise the rules they're labelled to exercise. This caused me to make minor modifications to some of the tests (that suffered from being solved via constant folding rather than the intention).
Line 424: // nullif: handled by folding
> move to constant folding test
Moved the ones that are simplified there. I removed lines 431-435 entirely, since it's really a test that DISTINCT FROM is being rewritten correctly, and we already have coverage for that.
Line 583: // nullif: handled by rewrite to if and simplification
> converted to if and simplified
Done.
Line 584: RewritesOk("nullif(bool_col, bool_col)", rules, "NULL");
> Move into SimplifyConditionalsTest
I've not done this, yet.
These expressions rely on the conversion and then on both SimplifyConditioanlsRule and SimplifyDistinctFromRule. As such, I think it's confusing to put them in one of the tests that's specific to those rules. (As I was working on this, every time there was "rules" vs "rule" in the test, it took a little bit of deciphering to figure out what was going on.)
All that said, happy to move this whole test function into SimplifyConditionalsTest if you think it's better.
http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:
Line 90: public void ParserAnalysisError(String stmt, String expectedErrorString) {
> Parser does analyze.
Done
PS6, Line 1238: // wrong number of arguments
: ParserAnalysisError("select nullif()", "nullif() must be called with two arguments.");
: ParserAnalysisError("select nullif(col, col2, col3)",
: "nullif() must be called with two arguments.");
I removed these; they're now tested elsewhere.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
Patch Set 4: -Code-Review
(2 comments)
Thanks for the review, Alex! I'll update nullif() to be simpler and use equals() for the literal case as well for the next iteration.
http://gerrit.cloudera.org:8080/#/c/7829/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
Line 160: if (expr.getChildren().size() == 1) {
> Sorry, but I think these simplifications are not needed because they are al
Yep, you're right. I'll remove this code, update comments to indicate that these methods exist, but are handled elsewhere, and I'll probably keep the tests but move them into wherever the folding rule is tested.
Line 220: * Simplifies nullif(a, b) when a and b are identical
> Why is equals() not sufficient on its own? Two identical literals should al
I'm not sure what you mean by your suggestion "...deal with NullLiteral case specially in this rule".
There are two cases for simplification:
x.equals(y): return NULL [x/y don't have to be literals!]
x.isLiteral() && y.isLiteral() && !x.equals(y): return x
(Note that the comment on line 223-224 is poor; I'll fix.)
Expressing it the way those two cases are written is simpler than what I have now, so I'll update that. I was initially worried that x.equals(y) might return false if the types of x and y are different, but that can't happen here since the types are promoted to be compatible by the time we get here.
I thought about converting nullif(x,y) to if(x=y, null, x). The problem with that is that it won't capture the x.equals(y) case where x/y aren't literals. We don't simplify if(x=x, null, x) -> NULL.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
Patch Set 4: Code-Review+1
Carry +1 from Matt Jacobs.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7829/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
Line 160: if (expr.getChildren().size() == 1) {
> Yep, you're right. I'll remove this code, update comments to indicate that
Before you add those existing tests to ExprRewriteRulesTest, please take a look at expr-test.cc to see whether there is already sufficient coverage there (my suspicion is yes, but better to check). The entire expr-test.cc is run with and without constant folding.
Line 220: * Simplifies nullif(a, b) when a and b are identical
> I'm not sure what you mean by your suggestion "...deal with NullLiteral cas
Sorry I got confused by the nullif() semantics, my suggestion with special casing the NullLiteral does not make sense.
How about the following solution:
1. Convert nullif() to IF(x <=> y, x, NULL). Remove the BE implementation, leave the tests. Note that IF(x=y, NULL, x) is not a correct transformation because NULL=NULL -> NULL
2. Enhance the existing IF simplification to deal with the specific case above (you can use expr.equals() just in the same way you would have here).
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................
Patch Set 7:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
Line 49: * x is [not] distinct from x -> false [true]
this is handled in your new rule
http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
Line 1642: AnalysisError("select nullif(1,2,3)", "default.nullif() unknown");
Weird. We should consider fixing this later by perhaps adding a dummy nullif() signature to the builtins db.
I think there's some general thinking to be done about these kind of converted exprs, e.g. how can we make them appear in "show functions".
But not for this patch. Can you file a JIRA for this improvement?
http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 48: static class CountingRewriteRuleWrapper implements ExprRewriteRule {
static not needed
Line 58: if (expr != ret) {
single line
Line 128: // All specified rules must have fired at least once.
Explain why.
Line 312: RewritesOk("nullif(NULL, NULL)", rule, "NULL");
Did you check whether these new tests are not already addressed by expr-test.cc? For constant folding, it's generally preferable to add new tests in expr-test.cc to keep everything in one place.
The tests here usually deal with FE<->BE specific issues for constant folding, or with particularly tricky folding cases.
Line 579: public void TestDistinctFromSimplificationRule() throws AnalysisException {
TestSimplifyDistinctFromRule()
Line 603: * it can be further simplified via DistinctFromSimplificatinoRule.
SimplifyDistinctFromRule
Line 619: // highlights that nullif gets transformed; the test infrastructure
I don't get the part about the test infrastructure.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7829/3/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
PS3, Line 319: boolean ret = (rewritten instanceof BoolLiteral && ((BoolLiteral) rewritten).getValue());
> I assume you meant to delete this line
Done
http://gerrit.cloudera.org:8080/#/c/7829/3/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 436: RewritesOk("isnottrue(null)", rule, "TRUE"); RewritesOk("isfalse(true)", rule, "FALSE");
> nit next line
Done
PS3, Line 451: rules
> as we discussed, it'd be helpful to rename this
I just added a small comment here, to avoid a somewhat annoying rename. Happy to be overwritten, it would just introduce a lot of diff noise in this large test function.
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
......................................................................
Patch Set 3: Code-Review+1
(3 comments)
Looks good, thanks! Just a few small things. Clearly Alex still needs to review this :)
http://gerrit.cloudera.org:8080/#/c/7829/3/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
PS3, Line 319: boolean ret = (rewritten instanceof BoolLiteral && ((BoolLiteral) rewritten).getValue());
I assume you meant to delete this line
http://gerrit.cloudera.org:8080/#/c/7829/3/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 436: RewritesOk("isnottrue(null)", rule, "TRUE"); RewritesOk("isfalse(true)", rule, "FALSE");
nit next line
PS3, Line 451: rules
as we discussed, it'd be helpful to rename this
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying ifnull conditional.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5211: Simplifying ifnull conditional.
......................................................................
Patch Set 5:
Alex,
Thanks for your reviews.
I put together a new diff which ties to do the re-write. The unit test passes, but I'm just now kicking off the full tests.
That said, I've got two questions I could use your advice on:
* Am I right in thinking that I can now go and delete IFNULL code from the backend?
* I'm worried about the types of NullLiteral. i.e., when I do the re-write I don't have the types of expressions here. I'm worried there's nuance here I don't yet know about.
I also am beginning to suspect that I shouldn't be special-casing if. Instead, "x [not] distinct from x -> false [true]" should be its own rule. Then this would apply in a few more cases.
After this conversation, we also think that https://impala.incubator.apache.org/docs/build/html/topics/impala_conditional_functions.html is wrong about how it rewrites nullif. It uses "=" and not "<=>" and so is wrong about nulls. I'm happy to update the docs or file a JIRA.
Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No