impala-reviews mailing list archives

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

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

Patch Set 4:


Thanks for the detailed review. addressed review comments.
File fe/src/main/java/org/apache/impala/rewrite/

Line 1: // Licensed to the Apache Software Foundation (ASF) under one
> Apache license header

Line 15: // specific language governing permissions and limitations
> grammar, I suggest this correction:

Line 29: /**
> inline these calls for brevity

Line 42:   @Override
> Still not very accurate. Sorry for riding on this point, but precise docume
good suggestion. Didnt have to change a lot. thanks

Line 58:    * is a compatible IN predicate or equality predicate. Returns the transformed
expr or null
> combine into L 57
combined the earlier notIn() into single if. Logically the next step is to compare the LHS
expressions, hence kept it separate.

Line 62:     InPredicate inPred = null;
> children -> newInList?

Line 64:     if (child0 instanceof InPredicate) {
> style: we always use {} for if/else-if, except if the whole if+then fits in
added explicit {} for if-else

Line 75:     // other predicate can be OR predicate or IN predicate
> Please follow suggestions above on making the comment more accurate, in par

Line 79:       newInList.add(otherPred.getChild(1));
> single line

Line 84:       } else {
> Assignment+cast not needed
agreed. Done
File fe/src/main/java/org/apache/impala/rewrite/

Line 29:  * are also normalized so that <constant> is always on the right hand side.
> Adjust comment to reflect new behavior, also fix indentation while you are 

Line 34:  * 5 = id + 2 -> id + 2 = 5
> Add an additional example where the rhs is not a simple slotref

Line 42:     if (!(expr instanceof BinaryPredicate)) return expr;
> No need for this comment, the class comment and examples should cover this

Line 43:     if (!expr.getChild(0).isConstant() || expr.getChild(1).isConstant()) {
> space after "if", rework logic to not have an empty then block
Personally, I prefer a flow which is easy to understand, even at the expense of having an
empty then block. I changed the logic to a non empty if block. let me know, and I can keep
the version that is preferred.
File fe/src/test/java/org/apache/impala/analysis/

Line 360:     RewritesOk("5 + 3 = id", rule, "id = 5 + 3");
> long line

Line 415:     RewritesOk("int_col = 1 or int_col in (2, 3)", edToInrule,
> add negative test cases:

Line 422:     RewritesOk("int_col = 1 or int_col = int_col ", edToInrule, null);
> Please be consistent with regards to capitalization of keywords in your tes

To view, visit
To unsubscribe, visit

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

View raw message