impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (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 18:26:16 GMT
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 <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