Zach Amsden has posted comments on this change. Change subject: IMPALA-5003: Generic constant propagation in planner ......................................................................
Patch Set 3: (17 comments) No longer a draft. Now with tests! 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 ca I can incorporate this next round if you want, but this diff didn't introduce it as a member. 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? Done 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 44: public final class ConstantPropagator { > brief class comment Done Line 51: //public static ConstantPropagator INSTANCE = new ConstantPropogator(); > remove Done PS3, Line 54: > comment on thrown exception and state of conjuncts in case of error fixed 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 f Done Line 63: while (changed) { > brief comment, maybe with a simple example to show why we need to do this r Done Line 80: try { > no need for try/catch if we're just going to throw e anyway Done Line 87: output.set(i, rewritten); > Seems cleaner to only do this when there was a change. That's how our expr Done Line 90: if (changed) Collections.copy(conjuncts, output); > Move this outside of the loop. You can iterate over output in L65 Done Line 95: // Propogates constants, removes duplicates and optionally performs expr rewriting > Mention that errors in the rewrite are logged, but ignored, and the 'conjun Done Line 96: public static void optimizeConjuncts(List<Expr> conjuncts, Analyzer analyzer, > instead of creating a class, why not just move this as static functions int Expr is already a pretty long class and is more of a utility classes for expr than a policy or implementation class for any optimizations. I didn't want aggressive optimization specific functions to start infecting Expr. I also thought maybe we could try optimizing (SR1 <= K, SR2 <= SR1 -> SR2 <= K) (and >= as well, this propagation is valid for any total ordering, but not >,< ) in the future. If we do that, this class would probably more warrant standing on its own. Doing so required more machinery in Expr matching than we currently have so I didn't try it in this diff. Line 101: for (int i = 0; i < conjuncts.size(); ++i) { > rewriter.rewriteList() Done Line 107: catch (AnalysisException e) { > goes after the } in L106 Done Line 108: LOG.error("Not able to analyze after rewrite: " + e.toString()); > LOG.warn and print the conjuncts in this message Done 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 crea Let me see if that comes out cleaner. 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 Agreed. Reduced this to two sets. -- 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: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
