You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2017/06/07 23:24:37 UTC
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
sakinapelli@cloudera.com has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7110
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
This change introduces a new rule to merge disjunct equality
predicates into an IN predicate. As with every rule being applied
bottom up, the rule merges the leaf OR predicates into an in predicate
and subsequently merges the OR predicate to the existing IN predicate
Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
A fe/src/main/java/org/apache/impala/rewrite/CoalesceEqualityDisjunctsToInRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
4 files changed, 156 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/7110/1
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sakinapelli@cloudera.com
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "sandeep akinapelli (Code Review)" <ge...@cloudera.org>.
sandeep akinapelli has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 5:
(4 comments)
addressed review comments
http://gerrit.cloudera.org:8080/#/c/7110/4/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java:
Line 30: * Coalesces disjunctive equality predicates to an IN predicate, and merges compatible
> Coalesces ... IN predicate, and ...
Done
Line 57: * The transformation is applied if one of the children is an IN predicate and the other child
> remove first comma
Done
Line 80: } else if (otherPred instanceof InPredicate && !((InPredicate) otherPred).isNotIn()) {
> else if
Done
http://gerrit.cloudera.org:8080/#/c/7110/3/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
File fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java:
Line 43:
> Fair point about the logic being more comprehensible. To make us both happy
Good suggestion. Done. the function is called isConstantOpExpr
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 2:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/7110/2/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
Line 180: public final static com.google.common.base.Predicate<Expr> IS_EXPR_EQ_LITERAL_PREDICATE = new com.google.common.base.Predicate<Expr>() {
long line
http://gerrit.cloudera.org:8080/#/c/7110/1/fe/src/main/java/org/apache/impala/rewrite/CoalesceEqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/CoalesceEqualityDisjunctsToInRule.java:
Line 69
> Agree. do we capture this effort in a separate jira?
I think this particular case is easy enough to fix in this JIRA. It's a simple generalization of the existing NormalizeBinaryPredicatesRule (always move constant exprs to the right)
http://gerrit.cloudera.org:8080/#/c/7110/2/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java:
Line 12: * This rule will coalesce multiple equality predicates to an IN predicate. It
Suggest modifications for brevity and grammar:
Coalesces disjunctive equality predicates to an IN predicate, and merges compatible equality predicates into existing IN predicates.
Line 16: * (X+Y = 5) OR (X+Y = 6) -> X+Y IN (5, 6)
add an example where an equality predicate is merged into an IN predicate
Line 32: if (orChildExpr != null) return orChildExpr;
What about IN OR IN? That can be produced by (a = 1 or a = 2) or (a = 3 or a = 4) and should result in a IN (1,2,3,4)
Line 38: * returns a rewritten expression for expression of type A= 1 OR A IN (2, 3)
This comment should describe the inputs and outputs more clearly, in particular, what happens when no rewrite is performed.
Line 52: if (!Expr.IS_EXPR_EQ_LITERAL_PREDICATE.apply(otherPred)) return null;
extra space before "return"
Line 54:
can get rid of these empty lines imo
Line 55: inPred.addChild(otherPred.getChild(1));
Our rewriting framework requires that we return a new InPredicate here. See the comment on ExprRewriteRule.apply().
Line 61: * returns a rewritten expression for expression of type A=1 OR A=2
Same here. Comment needs polish to describe inputs and outputs.
Line 64: if (!Expr.IS_EXPR_EQ_LITERAL_PREDICATE.apply(child0))
single lines?
Line 72: Expr leftChild0 = bp0.getChild(0);
Seems more concise to inline these function calls, and get rid of the extra assignments.
Line 78: if (!leftChild1.isLiteral()) return null;
To keep things simple, I think we should also require that rightChild1 is a literal. Otherwise, this rule is somewhat asymmetric.
http://gerrit.cloudera.org:8080/#/c/7110/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 392: RewritesOk("int_col = 1 or int_col = 2 or int_col = 3 and int_col = 4",
add parenthesis to make the AND/OR precedence explicit
Line 394:
remove blank lines, these 3 test cases belong together logically, so it's good to cluster them visually
Line 403: // combo rules
add negative tests to show the non-obvious cases in which the rewrite is not performed, for example:
int_col = smallint_col or int_col = bigint_col
Line 411: RewritesOk("int_col = 1 or int_col in (2, 3)", edToInrule,
int_col in (1,2) or int_col in (3,4) should also work
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sakinapelli@cloudera.com
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sakinapelli@cloudera.com
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 9: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 3:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/7110/3/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java:
Line 1: package org.apache.impala.rewrite;
Apache license header
Line 15: * merges the compatible equality or IN predicate into existing IN predicate.
grammar, I suggest this correction:
merges compatible equality or IN predicates into an existing IN predicate
Line 29: Expr child0 = expr.getChild(0);
inline these calls for brevity
Line 42: * Takes in two predicates, one is of type Expr IN (1, 2) and the other is of
Still not very accurate. Sorry for riding on this point, but precise documentation is important. Here are a few points that could be improved:
* The inputs can be any boolean expression, but the comment makes it sound like the caller must ensure that one of them is an IN predicate and the other is an IN predicate or equality predicate.
* Do not use examples to describe the function semantics. It's fine to use examples to support the description of this function, but the description itself should not be based on examples.
Consider this alternative formulation:
Takes the children of an OR predicate and attempts to combine them into a single IN predicate. The transformation is applied if one of the children is an IN predicate and the other child is a compatible IN predicate or equality predicate. Returns the transformed expr or null if no transformation was possible.
Line 58: if (inPred.isNotIn()) return null;
combine into L 57
Line 62: List<Expr> children = Lists.newArrayList(
children -> newInList?
Line 64: if (Expr.IS_EXPR_EQ_LITERAL_PREDICATE.apply(otherPred))
style: we always use {} for if/else-if, except if the whole if+then fits into a single line
Line 75: * Takes in two predicates of type 'Expr = literal' and returns the rewritten IN predicate.
Please follow suggestions above on making the comment more accurate, in particular, this function does not require child0/1 to be of the form <expr> = <literal>
Line 79: if (!Expr.IS_EXPR_EQ_LITERAL_PREDICATE.apply(child0))
single line
Line 84: BinaryPredicate bp0 = (BinaryPredicate) child0;
Assignment+cast not needed
http://gerrit.cloudera.org:8080/#/c/7110/3/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
File fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java:
Line 29: * Normalizes binary predicates of the form <expr> <op> <slot> so that the slot is
Adjust comment to reflect new behavior, also fix indentation while you are here.
Line 34: * 5 > id -> id < 5
Add an additional example where the rhs is not a simple slotref
Line 42: // always rewrite if LHS is literal and RHS is not a literal
No need for this comment, the class comment and examples should cover this
Line 43: if(expr.getChild(0).isLiteral() && !expr.getChild(1).isLiteral()) {}
space after "if", rework logic to not have an empty then block
for this rule we should check isConstant() instead of isLiteral() because this rule is required even with enable_expr_rewrites=false (which disables constant folding)
http://gerrit.cloudera.org:8080/#/c/7110/3/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 360: RewritesOk("tinyint_col + smallint_col = int_col", rule, "int_col = tinyint_col + smallint_col");
long line
Line 415: RewritesOk("int_col IN (1, 2) or int_col IN (3, 4)", edToInrule,
add negative test cases:
* lhs is NOT IN
* rhs is NOT IN
* lhs and rhs are NOT IN
Line 422: RewritesOk("int_col IN (1, 2) or int_col = int_col + 3 ", edToInrule, null);
Please be consistent with regards to capitalization of keywords in your test cases. Personally, I prefer to use lower caps in test cases so they can be quickly distinguished from the expected result (which is in caps).
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "sandeep akinapelli (Code Review)" <ge...@cloudera.org>.
sandeep akinapelli has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 4:
(17 comments)
Thanks for the detailed review. addressed review comments.
http://gerrit.cloudera.org:8080/#/c/7110/3/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java:
Line 1: // Licensed to the Apache Software Foundation (ASF) under one
> Apache license header
Done
Line 15: // specific language governing permissions and limitations
> grammar, I suggest this correction:
Done
Line 29: /**
> inline these calls for brevity
Done
Line 42: @Override
> Still not very accurate. Sorry for riding on this point, but precise docume
good suggestion. Didnt have to change a lot. thanks
Line 58: * is a compatible IN predicate or equality predicate. Returns the transformed expr or null
> combine into L 57
combined the earlier notIn() into single if. Logically the next step is to compare the LHS expressions, hence kept it separate.
Line 62: InPredicate inPred = null;
> children -> newInList?
Done
Line 64: if (child0 instanceof InPredicate) {
> style: we always use {} for if/else-if, except if the whole if+then fits in
added explicit {} for if-else
Line 75: // other predicate can be OR predicate or IN predicate
> Please follow suggestions above on making the comment more accurate, in par
Done
Line 79: newInList.add(otherPred.getChild(1));
> single line
Done
Line 84: } else {
> Assignment+cast not needed
agreed. Done
http://gerrit.cloudera.org:8080/#/c/7110/3/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
File fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java:
Line 29: * are also normalized so that <constant> is always on the right hand side.
> Adjust comment to reflect new behavior, also fix indentation while you are
Done
Line 34: * 5 = id + 2 -> id + 2 = 5
> Add an additional example where the rhs is not a simple slotref
Done
Line 42: if (!(expr instanceof BinaryPredicate)) return expr;
> No need for this comment, the class comment and examples should cover this
removed.
Line 43: if (!expr.getChild(0).isConstant() || expr.getChild(1).isConstant()) {
> space after "if", rework logic to not have an empty then block
Personally, I prefer a flow which is easy to understand, even at the expense of having an empty then block. I changed the logic to a non empty if block. let me know, and I can keep the version that is preferred.
http://gerrit.cloudera.org:8080/#/c/7110/3/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 360: RewritesOk("5 + 3 = id", rule, "id = 5 + 3");
> long line
Done
Line 415: RewritesOk("int_col = 1 or int_col in (2, 3)", edToInrule,
> add negative test cases:
Done
Line 422: RewritesOk("int_col = 1 or int_col = int_col ", edToInrule, null);
> Please be consistent with regards to capitalization of keywords in your tes
Done
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 9:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/807/
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "sandeep akinapelli (Code Review)" <ge...@cloudera.org>.
sandeep akinapelli has uploaded a new patch set (#5).
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
This change introduces a new rule to merge disjunct equality
predicates into an IN predicate. As with every rule being applied
bottom up, the rule merges the leaf OR predicates into an in predicate
and subsequently merges the OR predicate to the existing IN predicate
Patch also addresses review comments to
normalize the binary predicates and testcases for the same.
Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
A fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
5 files changed, 204 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/7110/5
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Re: [Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to
an IN predicate
Posted by Alexander Behm <al...@cloudera.com>.
A bunch of PlannerTests failed, please fix.
On Thu, Jun 15, 2017 at 10:20 PM, Impala Public Jenkins (Code Review) <
gerrit@cloudera.org> wrote:
> Impala Public Jenkins has posted comments on this change.
>
> Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN
> predicate
> ......................................................................
>
>
> Patch Set 7: Verified-1
>
> Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/739/
>
> --
> To view, visit http://gerrit.cloudera.org:8080/7110
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
> Gerrit-MessageType: comment
> Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
> Gerrit-PatchSet: 7
> Gerrit-Project: Impala-ASF
> Gerrit-Branch: master
> Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
> Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
> Gerrit-Reviewer: Impala Public Jenkins
> Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
> Gerrit-HasComments: No
>
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 7: Verified-1
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/739/
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "sandeep akinapelli (Code Review)" <ge...@cloudera.org>.
sandeep akinapelli has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 9:
(1 comment)
changed the testcase to include OR clause with another column
http://gerrit.cloudera.org:8080/#/c/7110/8/testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
File testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test:
Line 85: select * from functional_kudu.zipcode_incomes where id = '1' or id = '2' or zip = '3'
> I believe that was the intention of this test was to show that OR predicate
Done
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has uploaded a new patch set (#7).
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
This change introduces a new rule to merge disjunct equality
predicates into an IN predicate. As with every rule being applied
bottom up, the rule merges the leaf OR predicates into an in predicate
and subsequently merges the OR predicate to the existing IN predicate
It will also merge two compatible IN predicates into a single IN
predicate.
Patch also addresses review comments to
normalize the binary predicates and testcases for the same.
binary predicates of the form constant <op> non constant are normalized
to non constant <op> constant
Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
A fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
5 files changed, 204 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/7110/7
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "sandeep akinapelli (Code Review)" <ge...@cloudera.org>.
sandeep akinapelli has uploaded a new patch set (#4).
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
This change introduces a new rule to merge disjunct equality
predicates into an IN predicate. As with every rule being applied
bottom up, the rule merges the leaf OR predicates into an in predicate
and subsequently merges the OR predicate to the existing IN predicate
Patch also include changes in response to alex's review comments to
normalize the binary predicates and testcases for the same.
Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
A fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
5 files changed, 195 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/7110/4
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "sandeep akinapelli (Code Review)" <ge...@cloudera.org>.
sandeep akinapelli has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 8:
Fixed test cases by new checking in the new plan after OR to IN conversion.
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 1:
(21 comments)
Thanks for taking on this issue!
Comments should give you some ideas for additional test cases also. I'll take a closer look at the tests then.
http://gerrit.cloudera.org:8080/#/c/7110/1/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
Line 179: public final static com.google.common.base.Predicate<Expr> IS_EXPR_EQ_INCOMPATIBLE_PREDICATE = new com.google.common.base.Predicate<Expr>() {
Not really sure what this predicate does. Seems very specific, so probably better to move into the rewrite rule itself.
Line 183: && (((BinaryPredicate) arg).getChild(1) instanceof NumericLiteral
What's special about these literals? Why not all literals, i.e. isLiteral()?
http://gerrit.cloudera.org:8080/#/c/7110/1/fe/src/main/java/org/apache/impala/rewrite/CoalesceEqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/CoalesceEqualityDisjunctsToInRule.java:
Line 15: * This rule will coaelsce multiple equality preicates to an IN predicate
several typos
Line 16: * eg: (c=1) OR (c=2) OR (c=3) OR (c=4) -> c IN(1,2,3,4)
please follow the format of other rules to list examples
Line 19: public class CoalesceEqualityDisjunctsToInRule implements ExprRewriteRule {
CoalesceEqDisjunctsToInRule (a little shorter)
Line 21: private static final Logger LOG = Logger
remove, no logging please, too expensive
Line 27: LOG.info("Inside the apply clause ofCoalesceEqualityDisjunctsToInRule");
very expensive, remove
Line 28: if (!Expr.IS_OR_PREDICATE.apply(expr))
single line if
Line 34: if (inAndEqExpr != null) {
single line
Line 39: if (orChildExpr != null) {
single line
Line 46: /***
Try to be consistent with rest of code base:
/** (only two stars)
*
*/
Generally, avoid @param and @return for brevity
(please apply this comment to other functions as well)
Line 48: *
whitespace (same below)
Line 60: if (child1 instanceof InPredicate) {
else if seems clearer
Line 64: if (inPred == null) {
single line if
Line 67: if (!Expr.IS_EXPR_EQ_INCOMPATIBLE_PREDICATE.apply(otherPred))
Don't we need otherPred to be a BinaryPredicate with an EQ condition?
Line 69: if (!inPred.getChild(0).equals(otherPred.getChild(0))) {
Predicates might not be normalized, i.e. you could have
(10 = a+b) OR a+b IN (11)
Ideally, we'd normalize them properly, but that might be a larger endeavor. Maybe we can extend NormalizeBinaryPredicates to deal with those cases that are interesting for this rule.
Line 73: if (!inEle.getClass().equals(otherPred.getChild(1).getClass())) {
What is this check trying to do? It doesn't seem necessary.
Line 82: * rewrites predicates of type a=1 or a=2
We might want to extend NormalizeBinaryPredicates to make dealing with these cases easier. Not all kinds of BinaryPredicates are normalized unfortunately.
We should be able to deal with cases like this:
(a+b = 10) OR (11 = a+b)
Line 105: if (!leftChild1.getClass().equals(rightChild1.getClass())) {
What is this trying to do?
Line 109: if (!((leftChild1 instanceof NumericLiteral)
Why not leftChild1.isLiteral()?
http://gerrit.cloudera.org:8080/#/c/7110/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 80: public void TestCoalesceEqualityDisjunctsToInRule() throws AnalysisException {
nit: move to bottom
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sakinapelli@cloudera.com
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "sandeep akinapelli (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7110
to look at the new patch set (#9).
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
This change introduces a new rule to merge disjunct equality
predicates into an IN predicate. As with every rule being applied
bottom up, the rule merges the leaf OR predicates into an in predicate
and subsequently merges the OR predicate to the existing IN predicate
It will also merge two compatible IN predicates into a single IN
predicate.
Patch also addresses review comments to
normalize the binary predicates and testcases for the same.
binary predicates of the form constant <op> non constant are normalized
to non constant <op> constant
Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
A fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-views.test
13 files changed, 284 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/7110/9
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 4:
(4 comments)
We're almost done here, nice work!
http://gerrit.cloudera.org:8080/#/c/7110/4/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java:
Line 30: * Coalesce disjunctive equality predicates to an IN predicate,
Coalesces ... IN predicate, and ...
Line 57: * The transformation is applied, if one of the children is an IN predicate and the other child
remove first comma
Line 80: } else
else if
http://gerrit.cloudera.org:8080/#/c/7110/3/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
File fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java:
Line 43: if (!expr.getChild(0).isConstant() || expr.getChild(1).isConstant()) {
> Personally, I prefer a flow which is easy to understand, even at the expens
Fair point about the logic being more comprehensible. To make us both happy how about you add helper functions for the cases, e.g.:
boolean isExprOpConstant(Expr e) {
return !e.getChild(0).isConstant() && e.getChild(1).isConstant();
}
boolean isExprOpSlotRef() {
return expr.getChild(0).unwrapSlotRef(false) == null && expr.getChild(1).unwrapSlotRef(false) != null;
}
if (isExprOpSlotRef(expr) || isExprOpConstant()) {
// Do the trnasofrmation
}
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 7:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/739/
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "sandeep akinapelli (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7110
to look at the new patch set (#6).
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
This change introduces a new rule to merge disjunct equality
predicates into an IN predicate. As with every rule being applied
bottom up, the rule merges the leaf OR predicates into an in predicate
and subsequently merges the OR predicate to the existing IN predicate
It will also merge two compatible IN predicates into a single IN
predicate.
Patch also addresses review comments to
normalize the binary predicates and testcases for the same.
binary predicates of the form constant <op> non constant are normalized
to non constant <op> constant
Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
A fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
5 files changed, 204 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/7110/6
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
sakinapelli@cloudera.com has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 2:
(21 comments)
Responding to comments...
http://gerrit.cloudera.org:8080/#/c/7110/1/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
Line 179:
> Not really sure what this predicate does. Seems very specific, so probably
changed it to expr with equality condition and literal on the right. Generic enough to keep it here.
Line 183: return arg instanceof BinaryPredicate
> What's special about these literals? Why not all literals, i.e. isLiteral()
Done. bool and null has only few values. however if there are predicates then we dont want to restrict rewriting. hence chaged to isLiteral
http://gerrit.cloudera.org:8080/#/c/7110/1/fe/src/main/java/org/apache/impala/rewrite/CoalesceEqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/CoalesceEqualityDisjunctsToInRule.java:
Line 15
> several typos
Done
Line 16
> please follow the format of other rules to list examples
Done
Line 19
> CoalesceEqDisjunctsToInRule (a little shorter)
Done
Line 21
> remove, no logging please, too expensive
Done
Line 27
> very expensive, remove
Done
Line 28
> single line if
Done
Line 34
> single line
Done
Line 39
> single line
Done
Line 46
> Try to be consistent with rest of code base:
Done
Line 48
> whitespace (same below)
Done
Line 60
> else if seems clearer
Done
Line 64
> single line if
Done
Line 67
> Don't we need otherPred to be a BinaryPredicate with an EQ condition?
Added the condition in Expr.java
Line 69
> Predicates might not be normalized, i.e. you could have
Agree. do we capture this effort in a separate jira?
Line 73
> What is this check trying to do? It doesn't seem necessary.
Redundant. Trying to check if the elements in the IN clause and the candidate predicate literal is of same type or not. Removed.
Line 82
> We might want to extend NormalizeBinaryPredicates to make dealing with thes
Yes. added a combo testcase with normalized predicate rule for simple case.
Line 105
> What is this trying to do?
Redundant. removed.
Line 109
> Why not leftChild1.isLiteral()?
Done
http://gerrit.cloudera.org:8080/#/c/7110/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 80: public void TestBetweenToCompoundRule() throws AnalysisException {
> nit: move to bottom
Done
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sakinapelli@cloudera.com
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sakinapelli@cloudera.com
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
sakinapelli@cloudera.com has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 3:
(17 comments)
additional tests and changes to normalize binary predicate rule
http://gerrit.cloudera.org:8080/#/c/7110/2/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
Line 180: public final static com.google.common.base.Predicate<Expr> IS_EXPR_EQ_LITERAL_PREDICATE =
> long line
Done
http://gerrit.cloudera.org:8080/#/c/7110/1/fe/src/main/java/org/apache/impala/rewrite/CoalesceEqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/CoalesceEqualityDisjunctsToInRule.java:
Line 69
> I think this particular case is easy enough to fix in this JIRA. It's a sim
Adding code in the rule class.
http://gerrit.cloudera.org:8080/#/c/7110/2/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java:
Line 12:
> Suggest modifications for brevity and grammar:
Done
Line 16: * Examples:
> add an example where an equality predicate is merged into an IN predicate
Done
Line 32: Expr inAndOtherExpr = rewriteInAndOtherExpr(child0, child1);
> What about IN OR IN? That can be produced by (a = 1 or a = 2) or (a = 3 or
Done. Added code to merge two IN predicates.
Line 38: return expr;
> This comment should describe the inputs and outputs more clearly, in partic
Done
Line 52: }
> extra space before "return"
Done
Line 54: inPred = (InPredicate) child1;
> can get rid of these empty lines imo
Done
Line 55: otherPred = child0;
> Our rewriting framework requires that we return a new InPredicate here. See
Added code to instantiate new class
Line 61: // other predicate can be OR predicate or IN predicate
> Same here. Comment needs polish to describe inputs and outputs.
Done
Line 64: if (Expr.IS_EXPR_EQ_LITERAL_PREDICATE.apply(otherPred))
> single lines?
Done
Line 72: }
> Seems more concise to inline these function calls, and get rid of the extra
Done
Line 78: private Expr rewriteEqEqPredicate(Expr child0, Expr child1) {
> To keep things simple, I think we should also require that rightChild1 is a
removed this as it is covered by the IS_EXPR_EQ_LITERAL_PREDICATE function
http://gerrit.cloudera.org:8080/#/c/7110/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 392: RewritesOk("int_col * 3 = 6 or int_col * 3 = 9 or int_col * 3 = 12",
> add parenthesis to make the AND/OR precedence explicit
Done
Line 394:
> remove blank lines, these 3 test cases belong together logically, so it's g
Done
Line 403: edToInrule, "int_col * 3 IN (6, 9) OR int_col * 3 <= 12");
> add negative tests to show the non-obvious cases in which the rewrite is no
Done
Line 411: RewritesOk("int_col in (1,2) or int_col = 3", edToInrule,
> int_col in (1,2) or int_col in (3,4) should also work
Done
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sakinapelli@cloudera.com
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sakinapelli@cloudera.com
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7110/8/testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
File testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test:
Line 85: select * from functional_kudu.zipcode_incomes where id = '1' or id = '2'
I believe that was the intention of this test was to show that OR predicates cannot be pushed to Kudu. Please adjust this test to still have an OR predicate that cannot be pushed to Kudu.
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
sakinapelli@cloudera.com has uploaded a new patch set (#3).
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
This change introduces a new rule to merge disjunct equality
predicates into an IN predicate. As with every rule being applied
bottom up, the rule merges the leaf OR predicates into an in predicate
and subsequently merges the OR predicate to the existing IN predicate
Patch also include changes in response to alex's review comments to
normalize the binary predicates.
Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
A fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
5 files changed, 172 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/7110/3
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sakinapelli@cloudera.com
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sakinapelli@cloudera.com
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "sandeep akinapelli (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7110
to look at the new patch set (#8).
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
This change introduces a new rule to merge disjunct equality
predicates into an IN predicate. As with every rule being applied
bottom up, the rule merges the leaf OR predicates into an in predicate
and subsequently merges the OR predicate to the existing IN predicate
It will also merge two compatible IN predicates into a single IN
predicate.
Patch also addresses review comments to
normalize the binary predicates and testcases for the same.
binary predicates of the form constant <op> non constant are normalized
to non constant <op> constant
Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
A fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-views.test
13 files changed, 280 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/7110/8
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
This change introduces a new rule to merge disjunct equality
predicates into an IN predicate. As with every rule being applied
bottom up, the rule merges the leaf OR predicates into an in predicate
and subsequently merges the OR predicate to the existing IN predicate
It will also merge two compatible IN predicates into a single IN
predicate.
Patch also addresses review comments to
normalize the binary predicates and testcases for the same.
binary predicates of the form constant <op> non constant are normalized
to non constant <op> constant
Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Reviewed-on: http://gerrit.cloudera.org:8080/7110
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
A fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-views.test
13 files changed, 284 insertions(+), 49 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Alex Behm: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "sandeep akinapelli (Code Review)" <ge...@cloudera.org>.
sandeep akinapelli has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 6:
(2 comments)
addressed comments.
http://gerrit.cloudera.org:8080/#/c/7110/5/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java:
Line 82: otherPred.getChildren().subList(1, otherPred.getChildren().size()));
> fix indentation
Done
http://gerrit.cloudera.org:8080/#/c/7110/5/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
File fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java:
Line 47: }
> remove else block and just return expr at the end
Done
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 5: Code-Review+2
(2 comments)
Nice! Please address the final comments, rebase and upload a new patch set, then I'll start the merge.
http://gerrit.cloudera.org:8080/#/c/7110/5/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java:
Line 82: otherPred.getChildren().subList(1, otherPred.getChildren().size()));
fix indentation
http://gerrit.cloudera.org:8080/#/c/7110/5/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
File fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java:
Line 47: } else {
remove else block and just return expr at the end
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
Patch Set 7: Code-Review+2
I fixed another indentation issue.
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
sakinapelli@cloudera.com has uploaded a new patch set (#2).
Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
......................................................................
IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
This change introduces a new rule to merge disjunct equality
predicates into an IN predicate. As with every rule being applied
bottom up, the rule merges the leaf OR predicates into an in predicate
and subsequently merges the OR predicate to the existing IN predicate
Patch also include changes in response to alex's review comments
Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
A fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
4 files changed, 144 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/7110/2
--
To view, visit http://gerrit.cloudera.org:8080/7110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sakinapelli@cloudera.com
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>