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/25 10:08:36 UTC
[Impala-ASF-CR] IMPALA-5016: Simplify coalesce() in SimplifyConditionalsRule and do static partition pruning with coalesce() function.
yu feng has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6990
Change subject: IMPALA-5016: Simplify coalesce() in SimplifyConditionalsRule and do static partition pruning with coalesce() function.
......................................................................
IMPALA-5016: Simplify coalesce() in SimplifyConditionalsRule and do static partition pruning with coalesce() function.
rewrite coalesce() with head-most nulls or constant values.
add rewrite rules based on partition metadata.
try to apply rules for coalesce() in hdfs partition purning.
Testing:
add some unit tests for SimplifyConditionalsRule with coalesce() function.
Change-Id: I0b57267ce68ef882d73120b5054603b72867c7f5
---
M fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java
A fe/src/main/java/org/apache/impala/rewrite/CoalescePartitionMetadataRule.java
A fe/src/main/java/org/apache/impala/rewrite/PartitionMetadataRule.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, 210 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/6990/1
--
To view, visit http://gerrit.cloudera.org:8080/6990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0b57267ce68ef882d73120b5054603b72867c7f5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: yu feng <hz...@corp.netease.com>
[Impala-ASF-CR] IMPALA-5016: Simplify coalesce() in SimplifyConditionalsRule and 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: Simplify coalesce() in SimplifyConditionalsRule and do static partition pruning with coalesce() function.
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/6990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I0b57267ce68ef882d73120b5054603b72867c7f5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: yu feng <hz...@corp.netease.com>
[Impala-ASF-CR] IMPALA-5016: Simplify coalesce() in SimplifyConditionalsRule and do static partition pruning with coalesce() function.
Posted by "yu feng (Code Review)" <ge...@cloudera.org>.
yu feng has restored this change.
Change subject: IMPALA-5016: Simplify coalesce() in SimplifyConditionalsRule and do static partition pruning with coalesce() function.
......................................................................
Restored
--
To view, visit http://gerrit.cloudera.org:8080/6990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: restore
Gerrit-Change-Id: I0b57267ce68ef882d73120b5054603b72867c7f5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: yu feng <hz...@corp.netease.com>
Re: [Impala-ASF-CR] IMPALA-5016: Simplify coalesce() in
SimplifyConditionalsRule and do static partition pruning with coalesce() function.
Posted by Alexander Behm <al...@cloudera.com>.
Hey Yu,
in general, it's very useful for a reviewer to have responses to comments,
even if they are just "Done".
The typical workflow is to prepare a new patch, respond to comments, and
then push your new patch to gerrit. No need to abandon in between.
Thanks for your continued effort :). Will review your new patch.
Alex
On Tue, May 30, 2017 at 8:33 AM, yu feng (Code Review) <ge...@cloudera.org>
wrote:
> yu feng has abandoned this change.
>
> Change subject: IMPALA-5016: Simplify coalesce() in
> SimplifyConditionalsRule and do static partition pruning with coalesce()
> function.
> ......................................................................
>
>
> Abandoned
>
> Simplify the implement and add some tests.
>
> --
> To view, visit http://gerrit.cloudera.org:8080/6990
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
> Gerrit-MessageType: abandon
> Gerrit-Change-Id: I0b57267ce68ef882d73120b5054603b72867c7f5
> 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: Simplify coalesce() in SimplifyConditionalsRule and 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: Simplify coalesce() in SimplifyConditionalsRule and do static partition pruning with coalesce() function.
......................................................................
Abandoned
Simplify the implement and add some tests.
--
To view, visit http://gerrit.cloudera.org:8080/6990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I0b57267ce68ef882d73120b5054603b72867c7f5
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: Simplify coalesce() in SimplifyConditionalsRule and 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: Simplify coalesce() in SimplifyConditionalsRule and do static partition pruning with coalesce() function.
......................................................................
Patch Set 1:
(8 comments)
High-level comments. I'll take a closer look when we converge on the correct high-level approach first.
http://gerrit.cloudera.org:8080/#/c/6990/1/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java
File fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java:
Line 88: private final ExprRewriter partitionMetadataRewriter;
I don't think any changes should be made to HdfsPartitionPruner for this patch. See my other comment on Expr.optimizeConjuncts()
http://gerrit.cloudera.org:8080/#/c/6990/1/fe/src/main/java/org/apache/impala/rewrite/CoalescePartitionMetadataRule.java
File fe/src/main/java/org/apache/impala/rewrite/CoalescePartitionMetadataRule.java:
Line 35: * Rewrites coalesce function using partition metadata, it can be applied before partition pruning.
Let's please integrate this into the SimplifyConditionalsRule. I don't see this rule as being any different, and there is code/work duplication among this rule and your changes in SimplifyConditionalsRule.
This rule is just another case of simplification.
This rule should be generally applicable to any expr, not just those used for partition pruning. However, this rewrite rule will cause some queries to break when applied to exprs not used for partition pruning, e.g.:
select coalesce(1, count(*) from t
might be incorrectly rewritten to:
select 1 from t
The existing SimplifyConditionalsRule already handles these cases correctly (and this new rewrite will also just work when integrated into SimplifyConditionalsRule).
Line 44: public CoalescePartitionMetadataRule(HdfsTable table, List<SlotId> partitionSlots) {
These do not need to be passed. You can get this information from a SlotRef expr by looking at its SlotDescriptor. The SlotDescriptor's parent is a TupleDescriptor which has a reference to the Table. That should help with integrating this code into SimplifyConditionalsRule.
Line 67: private boolean canRewriteWithExpr(Expr child) {
This check does not look quite right because it will return 'true' for something like 'year + NULL'. The fact that the expr is bound by partition slots and that those partition slots cannot return NULL, does not mean that the overall expr must be non-NULL. The overall expr can still be NULL. You can imagine other Exprs that may return NULL for a non-NULL input.
I think it would be better to keep this simple for now, and only rewrite if the child is a (possibly cast) SlotRef.
http://gerrit.cloudera.org:8080/#/c/6990/1/fe/src/main/java/org/apache/impala/rewrite/PartitionMetadataRule.java
File fe/src/main/java/org/apache/impala/rewrite/PartitionMetadataRule.java:
Line 29: * Base class for all Expr rewrite rules that need using partition metadata.
I don't think this is needed. We apply all rewrite rules before doing partition pruning for scans already.
See Expr.optimizeConjuncts() which gets called in SingleNodePlanner.createScanNode().
http://gerrit.cloudera.org:8080/#/c/6990/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
Line 80: private Expr simplifyIfFunctionExpr(FunctionCallExpr expr) {
simplifyIfFunction()
Line 101: private Expr simplifyCoalesceFunctionExpr(FunctionCallExpr expr) {
simplifyCoalesceFunction()
http://gerrit.cloudera.org:8080/#/c/6990/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 339: RewritesOk("coalesce(null, a, b)", rule, "coalesce(a, b)");
Needs tests for the partition SlotRef case.
--
To view, visit http://gerrit.cloudera.org:8080/6990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0b57267ce68ef882d73120b5054603b72867c7f5
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: Yes