Zach Amsden has posted comments on this change. Change subject: IMPALA-5003: [DRAFT] Generic constant propagation in planner ......................................................................
Patch Set 2: (12 comments) Thanks for the fast review! http://gerrit.cloudera.org:8080/#/c/6389/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 2776: LOG.trace("value transfer: to " + slotIds.first.toString() > nit: can we flip to/from Done. It's still not entirely clear which value is to and which is from, but I found the output more useful with both values. http://gerrit.cloudera.org:8080/#/c/6389/2/fe/src/main/java/org/apache/impala/analysis/ConstantPropagator.java File fe/src/main/java/org/apache/impala/analysis/ConstantPropagator.java: Line 46: private static ExprRewriter rewriter; > ExprRewriter is not thread-safe. No good to make it static. Done Line 51: List<ExprRewriteRule> rewrite_rules = Lists.newArrayList(); > Java style: rewriteRules Done Line 53: rewrite_rules.add(FoldConstantsRule.INSTANCE); > I think we want to re-run all the expr rewrites after constant propagation. I added a separate optional pass to do this and consolidated all the rules in one place. Line 63: public static void propagateConstants(List<Expr> l, Analyzer analyzer) > l -> conjuncts? Done Line 65: if (l == null) return; > why not make this a Precondition Done Line 66: int changes = 1; > boolean changed? Done Line 71: if (expr instanceof BinaryPredicate && > This is a debatable style thing, but in the rest of the FE code we typicall I like the negation style :) Line 84: ++changes; > Why not run the rewrites here directly. That way we skip over exprs that ha Done Line 85: LOG.info("Constant substitution from slot " + > Definitely don't want this at info level. Done Line 106: LOG.info("Constant folded result: " + rewritten.toSql()); > Definitely don't want this at info level. Done Line 111: for (int i = 0; i < output.size(); ++i) l.set(i, output.get(i)); > Collections.copy() Done -- 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: 2 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
