impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5003: [DRAFT] Generic constant propagation in planner
Date Tue, 14 Mar 2017 20:34:52 GMT
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 <zamsden@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message