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: Simplify coalesce() in SimplifyConditionalsRule and do static partition pruning with coalesce() function.
Date Thu, 25 May 2017 19:43:07 GMT
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 <hzfengyu@corp.netease.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message