impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5003: Constant propagation in scan nodes
Date Wed, 05 Apr 2017 23:00:27 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-5003: Constant propagation in scan nodes
......................................................................


Patch Set 15:

(12 comments)

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

Line 923:    * Propagates constant expressions of the form <slot ref> = <constant>
to
> Explain candidate param
Done


Line 924:    * other uses of slot ref in all expressions in conjuncts; returns a BitSet with
> in all expressions in conjuncts -> in the given conjnucts
Done


Line 942:         // Don't rewrite with our own substitution!  We may have already processed
> Explanation seems more complicated than necessary. Clearly it would be wron
Done


Line 960:    * Returns true if the conjuncts may be true and false if a contradiction has
> Shorter: Returns false if a contradiction has been implied, true otherwise.
Done


Line 980:           // applied by the rewriter.  We must also re-analyze result exprs to
> I think we can remove everything after "We must also re-analyze  ..." to ke
Done


Line 985:           if (index < 0) continue;
> Agree. Makes sense.
Done


Line 988:           // Reset Expr to force updating cost
> Re-analyze expr to add implicit casts, resolve function signatures, and com
I made this slightly simpler due to required indentation level


Line 998:           // because they can't be evaluated if expr rewriting is turned off.
> You should be able to do this with a PlannerTest. For example, look at Plan
Done


http://gerrit.cloudera.org:8080/#/c/6389/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:

Line 1266: 
> Looks like the constant propagation is applied even when expr rewrites are 
Done


http://gerrit.cloudera.org:8080/#/c/6389/15/testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test
File testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test:

Line 101: # XXX - this seems correct, but is this a legal transformation?
> I agree that implicit casts on both sides seem ok.
Done


Line 161: # We can optimize in some cases
> Instead of testing various predicate assignment scenarios, we should focus 
Done.  I haven't tried an extreme example, but I added a few crazy ones to the test.


http://gerrit.cloudera.org:8080/#/c/6389/15/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test
File testdata/workloads/functional-planner/queries/PlannerTest/kudu.test:

Line 127: ---- SCANRANGELOCATIONS
> Don't think we need the SCANRANGELOCATIONS and DISTRIBUTEDPLAN sections.
nope


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I79750a8edb945effee2a519fa3b8192b77042cb4
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <zamsden@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message