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: Generic constant propagation in planner
Date Fri, 17 Mar 2017 02:59:31 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-5003: Generic constant propagation in planner

Patch Set 3:


No longer a draft.  Now with tests!
File fe/src/main/java/org/apache/impala/analysis/

Line 79:     rewriter_ = ExprRewriter.createExprRewriter(
> I think we don't actually need the rewriter_ as a member, and instead we ca
I can incorporate this next round if you want, but this diff didn't introduce it as a member.
File fe/src/main/java/org/apache/impala/analysis/

Line 2777:                       + " to " + slotIds.second.toString());
> I'm not even sure this message helps at all. Remove?
File fe/src/main/java/org/apache/impala/analysis/

Line 44: public final class ConstantPropagator {
> brief class comment

Line 51:   //public static ConstantPropagator INSTANCE = new ConstantPropogator();
> remove

PS3, Line 54:   
> comment on thrown exception and state of conjuncts in case of error

Line 56:   public static boolean propagateConstants(List<Expr> conjuncts, Analyzer analyzer)
> make private, we can still make it public later when the need arises, but f

Line 63:     while (changed) {
> brief comment, maybe with a simple example to show why we need to do this r

Line 80:             try {
> no need for try/catch if we're just going to throw e anyway

Line 87:           output.set(i, rewritten);
> Seems cleaner to only do this when there was a change. That's how our expr 

Line 90:       if (changed) Collections.copy(conjuncts, output);
> Move this outside of the loop. You can iterate over output in L65

Line 95:   // Propogates constants, removes duplicates and optionally performs expr rewriting
> Mention that errors in the rewrite are logged, but ignored, and the 'conjun

Line 96:   public static void optimizeConjuncts(List<Expr> conjuncts, Analyzer analyzer,
> instead of creating a class, why not just move this as static functions int
Expr is already a pretty long class and is more of a utility classes for expr than a policy
or implementation class for any optimizations.  I didn't want aggressive optimization specific
functions to start infecting Expr.

I also thought maybe we could try optimizing (SR1 <= K, SR2 <= SR1 -> SR2 <= K)
(and >= as well, this propagation is valid for any total ordering, but not >,< )
in the future.

If we do that, this class would probably more warrant standing on its own.  Doing so required
more machinery in Expr matching than we currently have so I didn't try it in this diff.

Line 101:         for (int i = 0; i < conjuncts.size(); ++i) {
> rewriter.rewriteList()

Line 107:     catch (AnalysisException e) {
> goes after the } in L106

Line 108:       LOG.error("Not able to analyze after rewrite: " + e.toString());
> LOG.warn and print the conjuncts in this message
File fe/src/main/java/org/apache/impala/planner/

Line 1217:     ConstantPropagator.optimizeConjuncts(conjuncts, analyzer,
> To clean up this retrieval of enable_expr_rewrites, consider adding an crea
Let me see if that comes out cleaner.
File fe/src/main/java/org/apache/impala/rewrite/

Line 48:   public enum RuleSet {
> I see what you are doing, but I don't think we need to be quite as general 
Agreed.  Reduced this to two sets.

To view, visit
To unsubscribe, visit

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

View raw message