impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "sandeep akinapelli (Code Review)" <ger...@cloudera.org>
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:

(17 comments)

Thanks for the detailed review. addressed review comments.

http://gerrit.cloudera.org:8080/#/c/7110/3/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
File fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java:

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


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


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


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


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
Done


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


Line 84:       } else {
> Assignment+cast not needed
agreed. Done


http://gerrit.cloudera.org:8080/#/c/7110/3/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
File fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java:

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 
Done


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


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


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.


http://gerrit.cloudera.org:8080/#/c/7110/3/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

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


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


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
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sakinapelli@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sakinapelli@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message