Alex Behm has posted comments on this change. Change subject: IMPALA-5003: [DRAFT] Generic constant propagation in planner ......................................................................
Patch Set 2: (12 comments) Looks reasonable. 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 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. Line 51: List<ExprRewriteRule> rewrite_rules = Lists.newArrayList(); Java style: rewriteRules Line 53: rewrite_rules.add(FoldConstantsRule.INSTANCE); I think we want to re-run all the expr rewrites after constant propagation. We could either cram everything in here, or minimize the ConstantPropagator. I'd prefer the latter. Constant folding is not strictly necessary here, but normalization is since your the constant-propagation code relies on it. Line 63: public static void propagateConstants(List<Expr> l, Analyzer analyzer) l -> conjuncts? Line 65: if (l == null) return; why not make this a Precondition Line 66: int changes = 1; boolean changed? Line 71: if (expr instanceof BinaryPredicate && This is a debatable style thing, but in the rest of the FE code we typically invert such conditions and then 'continue' to avoid deep indentation. So in this case: Line 84: ++changes; Why not run the rewrites here directly. That way we skip over exprs that have not changed. Line 85: LOG.info("Constant substitution from slot " + Definitely don't want this at info level. Line 106: LOG.info("Constant folded result: " + rewritten.toSql()); Definitely don't want this at info level. Line 111: for (int i = 0; i < output.size(); ++i) l.set(i, output.get(i)); Collections.copy() -- 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
