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: sakinape...@cloudera.com Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-HasComments: Yes
