impala-reviews mailing list archives

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

Mime
View raw message