Alex Behm has posted comments on this change. Change subject: IMPALA-5003: [DRAFT] Generic constant propagation in planner ......................................................................
Patch Set 3: (17 comments) We still need tests. I suggest either adding a new unittest similar to ExprRewriteTest, or just adding the tests in there. http://gerrit.cloudera.org:8080/#/c/6389/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: Line 79: rewriter_ = ExprRewriter.createExprRewriter( I think we don't actually need the rewriter_ as a member, and instead we can create it on demand in analyze() below using the createExprRewriter() in Analyzer (see my other comment on adding this convenience function) http://gerrit.cloudera.org:8080/#/c/6389/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 2777: + " to " + slotIds.second.toString()); I'm not even sure this message helps at all. Remove? http://gerrit.cloudera.org:8080/#/c/6389/3/fe/src/main/java/org/apache/impala/analysis/ConstantPropagator.java File fe/src/main/java/org/apache/impala/analysis/ConstantPropagator.java: Line 1: extra newline Line 44: public final class ConstantPropagator { brief class comment Line 51: //public static ConstantPropagator INSTANCE = new ConstantPropogator(); remove PS3, Line 54: comment on thrown exception and state of conjuncts in case of error Line 56: public static boolean propagateConstants(List<Expr> conjuncts, Analyzer analyzer) make private, we can still make it public later when the need arises, but for now we only expect optimizeConjuncts() to be called Line 63: while (changed) { brief comment, maybe with a simple example to show why we need to do this repeatedly Line 80: try { no need for try/catch if we're just going to throw e anyway Line 87: output.set(i, rewritten); Seems cleaner to only do this when there was a change. That's how our expr rewrites generally work (returned expr == original expr, if there were no changes) Line 90: if (changed) Collections.copy(conjuncts, output); Move this outside of the loop. You can iterate over output in L65 Line 95: // Propogates constants, removes duplicates and optionally performs expr rewriting Mention that errors in the rewrite are logged, but ignored, and the 'conjuncts' remain unchanged then Line 101: for (int i = 0; i < conjuncts.size(); ++i) { rewriter.rewriteList() Line 107: catch (AnalysisException e) { goes after the } in L106 Line 108: LOG.error("Not able to analyze after rewrite: " + e.toString()); LOG.warn and print the conjuncts in this message http://gerrit.cloudera.org:8080/#/c/6389/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: Line 1217: ConstantPropagator.optimizeConjuncts(conjuncts, analyzer, To clean up this retrieval of enable_expr_rewrites, consider adding an createExprRewriter() function in Analyzer. http://gerrit.cloudera.org:8080/#/c/6389/3/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java File fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java: Line 48: public enum RuleSet { I see what you are doing, but I don't think we need to be quite as general yet. The ExprRewriter framework is relatively new and there are still many missing rules. I'd like to keep it as simple as possible to add new rules and make modifications to how/where rules are applied. The downside of the current approach is that rule implementers need to be aware of all the different uses of the ExprRewriter to pick the set of RuleSets it should be added to. How about we just keep the two original rule sets? The required ones and then required + all others. I understand we may sometimes be invoking rules without a hope of them firing. If excessive rewrites attempts become a perf problem, we can still revisit. -- 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: 3 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: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
