sakinape...@cloudera.com has posted comments on this change. Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate ......................................................................
Patch Set 2: (21 comments) Responding to comments... 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: > Not really sure what this predicate does. Seems very specific, so probably changed it to expr with equality condition and literal on the right. Generic enough to keep it here. Line 183: return arg instanceof BinaryPredicate > What's special about these literals? Why not all literals, i.e. isLiteral() Done. bool and null has only few values. however if there are predicates then we dont want to restrict rewriting. hence chaged to 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 > several typos Done Line 16 > please follow the format of other rules to list examples Done Line 19 > CoalesceEqDisjunctsToInRule (a little shorter) Done Line 21 > remove, no logging please, too expensive Done Line 27 > very expensive, remove Done Line 28 > single line if Done Line 34 > single line Done Line 39 > single line Done Line 46 > Try to be consistent with rest of code base: Done Line 48 > whitespace (same below) Done Line 60 > else if seems clearer Done Line 64 > single line if Done Line 67 > Don't we need otherPred to be a BinaryPredicate with an EQ condition? Added the condition in Expr.java Line 69 > Predicates might not be normalized, i.e. you could have Agree. do we capture this effort in a separate jira? Line 73 > What is this check trying to do? It doesn't seem necessary. Redundant. Trying to check if the elements in the IN clause and the candidate predicate literal is of same type or not. Removed. Line 82 > We might want to extend NormalizeBinaryPredicates to make dealing with thes Yes. added a combo testcase with normalized predicate rule for simple case. Line 105 > What is this trying to do? Redundant. removed. Line 109 > Why not leftChild1.isLiteral()? Done 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 TestBetweenToCompoundRule() throws AnalysisException { > nit: move to bottom 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: sakinape...@cloudera.com Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: sakinape...@cloudera.com Gerrit-HasComments: Yes
