impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5003: [DRAFT] Generic constant propagation in planner
Date Tue, 14 Mar 2017 23:45:50 GMT
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 <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