[email protected] 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: [email protected] Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes
