impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Date Mon, 12 Jun 2017 17:41:26 GMT
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 <alex.behm@cloudera.com>
Gerrit-Reviewer: sakinapelli@cloudera.com
Gerrit-HasComments: Yes

Mime
View raw message