impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5280: Coalesce chains of OR conditions to an IN predicate
Date Mon, 12 Jun 2017 17:41:26 GMT
Alex Behm has posted comments on this change.

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

Patch Set 2:

File fe/src/main/java/org/apache/impala/analysis/

Line 180:   public final static<Expr> IS_EXPR_EQ_LITERAL_PREDICATE
= new<Expr>() {
long line
File fe/src/main/java/org/apache/impala/rewrite/

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

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

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

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

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
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