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-5016 rewrite coalesce() in SimplifyConditionalsRule.java, and try to do static partition pruning with coalesce() function.
Date Wed, 24 May 2017 19:27:52 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-5016 rewrite coalesce() in SimplifyConditionalsRule.java, and try to
do static partition pruning with coalesce() function.
......................................................................


Patch Set 1:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/6974/1//COMMIT_MSG
Commit Message:

Line 7: IMPALA-5016 rewrite coalesce() in SimplifyConditionalsRule.java,
We typically follow this format for commit msgs:

IMPALA-ABCD: Single-line summary of change.
<blank line>
More detailed description of change.
<blank line>
Testing:
Describe what tests you added, what testing you did.

So how about something like this:

IMPALA-5016: Simplify coalesce() in SimplifyConditionalsRule.


http://gerrit.cloudera.org:8080/#/c/6974/1/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

Line 189:   public final static com.google.common.base.Predicate<Expr>
Don't think this is necessary. Please inline into the RewriteRule.


http://gerrit.cloudera.org:8080/#/c/6974/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

Line 242:   public boolean isCoalesceBuiltinFn() {
Don't think this is necessary. Please inline into the RewriteRule.


http://gerrit.cloudera.org:8080/#/c/6974/1/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java
File fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java:

Line 129:       } else if(conjunct.contains(Expr.IS_COALESCE_BUILTIN_FN_PREDICATE)) {
I think this should be handled in the rewrite rule and not in this code. Do you have a case
in mind that we would not be able to handle in the rewrite rule?

I'm thinking the transformation would be like this:

coalesce(a, b, c) -> a

if 'a' is a SlotRef to a non-nullable column
or
if 'a' is a SlotRef to a partition column and that partition column does not have the NULL
value

In particular, I'm concerned about the added complexity to this code for handling a very narrow
case.


http://gerrit.cloudera.org:8080/#/c/6974/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:

Line 85:       Preconditions.checkState(expr.getChildren().size() == 3);
please move these cases into their own functions, i.e., 
simplifyIf()
simplifyCoalesce()


Line 100:       for(int i = 0; i < childrenSize; ++ i) {
Please try to follow the existing style:

space after "for", no space after "++"


Line 103:         if(!childExpr.isConstant()) {
I believe the rewrite is subtly wrong because:
coalesce(1 + NULL, 10) -> coalesce(1 + NULL)

One fix is to ignore non-literal constant exprs, and rely on constant folding to correctly
simplify those cases.


Line 104:           List<Expr> newChildren = Lists.newArrayList(expr.getChildren().subList(i,
childrenSize));
1. We should try very hard not to generate objects when no rewrite is performed.
2. When no rewrite is performed, we should return the original unmodified expr.


Line 110:           return expr;
Please look at the comment on ExprRewriteRule.apply(). Returning expr here communicates to
the caller that no rewrite was performed, when in fact, you did modify the expr.

The general rule is that when a rewrite was performed you need to return a new expression.


Line 113:         if(!childExpr.isNullLiteral()) {
style: space after "if" and single line if possible


-- 
To view, visit http://gerrit.cloudera.org:8080/6974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I00aac2497ba51432037a6f9312805a63933ace87
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: yu feng <hzfengyu@corp.netease.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message