Alex Behm has posted comments on this change. Change subject: IMPALA-5280: Coalesce chains of OR conditions to an IN predicate ......................................................................
Patch Set 3: (17 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: package org.apache.impala.rewrite; Apache license header Line 15: * merges the compatible equality or IN predicate into existing IN predicate. grammar, I suggest this correction: merges compatible equality or IN predicates into an existing IN predicate Line 29: Expr child0 = expr.getChild(0); inline these calls for brevity Line 42: * Takes in two predicates, one is of type Expr IN (1, 2) and the other is of Still not very accurate. Sorry for riding on this point, but precise documentation is important. Here are a few points that could be improved: * The inputs can be any boolean expression, but the comment makes it sound like the caller must ensure that one of them is an IN predicate and the other is an IN predicate or equality predicate. * Do not use examples to describe the function semantics. It's fine to use examples to support the description of this function, but the description itself should not be based on examples. Consider this alternative formulation: Takes the children of an OR predicate and attempts to combine them into a single IN predicate. The transformation is applied if one of the children is an IN predicate and the other child is a compatible IN predicate or equality predicate. Returns the transformed expr or null if no transformation was possible. Line 58: if (inPred.isNotIn()) return null; combine into L 57 Line 62: List<Expr> children = Lists.newArrayList( children -> newInList? Line 64: if (Expr.IS_EXPR_EQ_LITERAL_PREDICATE.apply(otherPred)) style: we always use {} for if/else-if, except if the whole if+then fits into a single line Line 75: * Takes in two predicates of type 'Expr = literal' and returns the rewritten IN predicate. Please follow suggestions above on making the comment more accurate, in particular, this function does not require child0/1 to be of the form <expr> = <literal> Line 79: if (!Expr.IS_EXPR_EQ_LITERAL_PREDICATE.apply(child0)) single line Line 84: BinaryPredicate bp0 = (BinaryPredicate) child0; Assignment+cast not needed 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: * Normalizes binary predicates of the form <expr> <op> <slot> so that the slot is Adjust comment to reflect new behavior, also fix indentation while you are here. Line 34: * 5 > id -> id < 5 Add an additional example where the rhs is not a simple slotref Line 42: // always rewrite if LHS is literal and RHS is not a literal No need for this comment, the class comment and examples should cover this Line 43: if(expr.getChild(0).isLiteral() && !expr.getChild(1).isLiteral()) {} space after "if", rework logic to not have an empty then block for this rule we should check isConstant() instead of isLiteral() because this rule is required even with enable_expr_rewrites=false (which disables constant folding) 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("tinyint_col + smallint_col = int_col", rule, "int_col = tinyint_col + smallint_col"); long line Line 415: RewritesOk("int_col IN (1, 2) or int_col IN (3, 4)", edToInrule, add negative test cases: * lhs is NOT IN * rhs is NOT IN * lhs and rhs are NOT IN Line 422: RewritesOk("int_col IN (1, 2) or int_col = int_col + 3 ", edToInrule, null); Please be consistent with regards to capitalization of keywords in your test cases. Personally, I prefer to use lower caps in test cases so they can be quickly distinguished from the expected result (which is in caps). -- 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: 3 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
