impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <>
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:


Thanks for the fast review!
File fe/src/main/java/org/apache/impala/analysis/

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.
File fe/src/main/java/org/apache/impala/analysis/

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.
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?

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 typicall
I like the negation style :)

Line 84:                 ++changes;
> Why not run the rewrites here directly. That way we skip over exprs that ha

Line 85:       "Constant substitution from slot " +
> Definitely don't want this at info level.

Line 106:   "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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I79750a8edb945effee2a519fa3b8192b77042cb4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Zach Amsden <>
Gerrit-HasComments: Yes

View raw message