impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anonymous Coward (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Date Sat, 10 Jun 2017 05:20:37 GMT has posted comments on this change.

Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate

Patch Set 2:


Responding to comments...
File fe/src/main/java/org/apache/impala/analysis/

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
File fe/src/main/java/org/apache/impala/rewrite/

Line 15
> several typos

Line 16
> please follow the format of other rules to list examples

Line 19
> CoalesceEqDisjunctsToInRule (a little shorter)

Line 21
> remove, no logging please, too expensive

Line 27
> very expensive, remove

Line 28
> single line if

Line 34
> single line

Line 39
> single line

Line 46
> Try to be consistent with rest of code base:

Line 48
> whitespace (same below)

Line 60
> else if seems clearer

Line 64
> single line if

Line 67
> Don't we need otherPred to be a BinaryPredicate with an EQ condition?
Added the condition in

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()?
File fe/src/test/java/org/apache/impala/analysis/

Line 80:   public void TestBetweenToCompoundRule() throws AnalysisException {
> nit: move to bottom

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: If02396b752c5497de9a92828c24c8062027dc2e2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Reviewer: Alex Behm <>
Gerrit-HasComments: Yes

View raw message