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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: sandeep akinapelli <[email protected]> Gerrit-HasComments: Yes
