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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
