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 Sat, 10 Jun 2017 05:20:37 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 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: sakinapelli@cloudera.com
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: sakinapelli@cloudera.com
Gerrit-HasComments: Yes

Mime
View raw message