impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <>
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:

File fe/src/main/java/org/apache/impala/analysis/

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

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

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

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.

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

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

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
File fe/src/main/java/org/apache/impala/planner/

Line 1266: 
> Looks like the constant propagation is applied even when expr rewrites are 
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.

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.
File testdata/workloads/functional-planner/queries/PlannerTest/kudu.test:

> Don't think we need the SCANRANGELOCATIONS and DISTRIBUTEDPLAN sections.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I79750a8edb945effee2a519fa3b8192b77042cb4
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Zach Amsden <>
Gerrit-HasComments: Yes

View raw message