You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "yu feng (Code Review)" <ge...@cloudera.org> on 2017/05/24 12:39:57 UTC
[Impala-ASF-CR] IMPALA-5016 rewrite coalesce() in SimplifyConditionalsRule.java, and try to do static partition pruning with coalesce() function.
yu feng has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6974
Change subject: IMPALA-5016 rewrite coalesce() in SimplifyConditionalsRule.java, and try to do static partition pruning with coalesce() function.
......................................................................
IMPALA-5016 rewrite coalesce() in SimplifyConditionalsRule.java,
and try to do static partition pruning with coalesce() function.
Change-Id: I00aac2497ba51432037a6f9312805a63933ace87
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
5 files changed, 104 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/6974/1
--
To view, visit http://gerrit.cloudera.org:8080/6974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I00aac2497ba51432037a6f9312805a63933ace87
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: yu feng <hz...@corp.netease.com>
[Impala-ASF-CR] IMPALA-5016 rewrite coalesce() in SimplifyConditionalsRule.java, and try to do static partition pruning with coalesce() function.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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:
No worries, the first patch is always the hardest :). Thanks for your fast response and continued work.
--
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 <hz...@corp.netease.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5016 rewrite coalesce() in SimplifyConditionalsRule.java, and try to do static partition pruning with coalesce() function.
Posted by "yu feng (Code Review)" <ge...@cloudera.org>.
yu feng has abandoned this change.
Change subject: IMPALA-5016 rewrite coalesce() in SimplifyConditionalsRule.java, and try to do static partition pruning with coalesce() function.
......................................................................
Abandoned
sorry for the bad format and I done all the changes in another review.
--
To view, visit http://gerrit.cloudera.org:8080/6974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I00aac2497ba51432037a6f9312805a63933ace87
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: yu feng <hz...@corp.netease.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
[Impala-ASF-CR] IMPALA-5016 rewrite coalesce() in SimplifyConditionalsRule.java, and try to do static partition pruning with coalesce() function.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <hz...@corp.netease.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-HasComments: Yes