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