Migrate ProjectionPushDown to iterative optimizer #7256

losipiuk · 2017-01-31T17:39:57Z

No description provided.

losipiuk · 2017-01-31T23:16:38Z

There is yet some test failure. I will investigate it tomorrow.

kokosing

minor comments unless you modified rules

kokosing · 2017-02-01T07:18:34Z

presto-main/src/test/java/com/facebook/presto/sql/planner/iterative/rule/test/PlanBuilder.java

@@ -78,7 +78,7 @@ public Symbol symbol(String name, Type type)
    {
        Symbol symbol = new Symbol(name);

-        Type old = symbols.get(symbol);
+        Type old = symbols.put(symbol, type);


put if symbols not contains

But we allow the symbol to already be in the map provided the type match.

Either retain get here or remove put 5 lines down the road.

kokosing · 2017-02-01T07:20:18Z

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/PlanMatchPattern.java

@@ -236,7 +237,8 @@ public static PlanMatchPattern join(JoinNode.Type joinType, List<ExpectedValuePr
                        expectedFilter.map(predicate -> rewriteQualifiedNamesToSymbolReferences(new SqlParser().createExpression(predicate)))));
    }

-    public static PlanMatchPattern exchange(PlanMatchPattern... sources) {
+    public static PlanMatchPattern exchange(PlanMatchPattern... sources)
+    {


as separate commit?

done already

kokosing · 2017-02-01T07:22:06Z

...java/com/facebook/presto/sql/planner/iterative/rule/test/TestPushProjectionThroughUnion.java

+                                // verify that data originally on symbols aliased as x1 and x2 is part of exchange output
+                                .withAlias("x1")
+                                .withAlias("x2"));
+        // todo check contents of values


maybe you could please add support for values. I see this TODO in so many rule tests that I am surprised that nobody fixed that so far.

I think it is done already if I understand the todo intention correctly

kokosing · 2017-02-01T07:23:53Z

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/AliasMatches.java

@@ -24,13 +24,13 @@
 import static java.lang.String.format;
 import static java.util.Objects.requireNonNull;

-public class Alias
+public class AliasMatches


AliasMatcher?

kokosing · 2017-02-01T07:24:56Z

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/AliasPresent.java

+    {
+        // just check alias is present
+        // return self mapping from symbolAliasesMap.
+        return symbolAliases.getOptional(alias).map(sr -> new Symbol(sr.getName()));


put .map(...) in separate line

kokosing · 2017-02-01T07:28:02Z

presto-main/src/test/java/com/facebook/presto/sql/planner/iterative/rule/test/PlanBuilder.java

@@ -124,7 +126,7 @@ public ExchangeBuilder addSource(PlanNode source)
            return this;
        }

-        public ExchangeBuilder addInputsSet(Symbol ... inputs)
+        public ExchangeBuilder addInputsSet(Symbol... inputs)


include this change in the commit which introduced this code

done already

so why do I still see this?

Weird. Annotate in IntelliJ showed last modification from older commit. Fixed.

kokosing · 2017-02-01T07:30:20Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughExchange.java

+        implements Rule
+{
+    @Override
+    public Optional<PlanNode> apply(PlanNode node, Lookup lookup, PlanNodeIdAllocator idAllocator, SymbolAllocator symbolAllocator)


did you have to logically modify the rule somehow?

modifications are in separate commit.

kokosing · 2017-02-01T07:33:13Z

presto-main/src/test/java/com/facebook/presto/sql/planner/iterative/rule/test/PlanBuilder.java

+            return this;
+        }
+
+        public ExchangeBuilder singleDistributionPartitioningScheme(Symbol... outputSymbols)


put it one method above

kokosing · 2017-02-01T07:34:09Z

...src/main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughUnion.java

+        return Optional.of(new UnionNode(node.getId(), outputSources.build(), mappings.build(), ImmutableList.copyOf(mappings.build().keySet())));
+    }
+
+    private static Expression translateExpression(Expression inputExpression, Map<Symbol, SymbolReference> symbolMapping)


maybe put it as static method of ExpressionSymbolInliner?

Let's leave it for now.

kokosing · 2017-02-01T07:34:22Z

...src/main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughUnion.java

+        implements Rule
+{
+    @Override
+    public Optional<PlanNode> apply(PlanNode node, Lookup lookup, PlanNodeIdAllocator idAllocator, SymbolAllocator symbolAllocator)


did you modify this rule?

losipiuk · 2017-02-02T15:34:32Z

Updated.
@kokosing, @martint please take a look.

It seems like followup of this work would be to introduce explicit-output symbols to ExchangeNode so we always can get rid of Projection on top of exchange and not send unnecessary symbols over the network.

kokosing

Please also address my comments from previous review

kokosing · 2017-02-03T07:19:49Z

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/AliasPresent.java

+    public Optional<Symbol> getAssignedSymbol(PlanNode node, Session session, Metadata metadata, SymbolAliases symbolAliases)
+    {
+        return symbolAliases.getOptional(alias)
+                .map(sr -> new Symbol(sr.getName()));


use Symbol::from

kokosing · 2017-02-03T07:21:08Z

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/SymbolAliases.java

@@ -81,6 +82,13 @@ public SymbolReference get(String alias)
        return result;


use your method here

what method?

use getOptional in implementation of get

kokosing · 2017-02-03T07:22:01Z

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/PlanMatchPattern.java

@@ -368,6 +369,11 @@ public PlanMatchPattern withNumberOfOutputColumns(int numberOfSymbols)
     * in the outputs. This is the case for symbols that are produced by a direct or indirect
     * source of the node you're applying this to.
     */
+    public PlanMatchPattern withExactOutputs(String... expectedAliases)
+    {
+        return withExactOutputs(asList(expectedAliases));


use ImmutableList

kokosing · 2017-02-03T07:30:09Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughExchange.java

+ * Transforms:
+ *
+ *  <pre>
+ *  PROJ(x = e1, y = e2)


use Project instead of PROJ.

Please do not use capitalized names. You do not need to shout. :)

Changed the casing. Not sure if that is actually more readable. Whatever.

kokosing · 2017-02-03T07:30:44Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughExchange.java

@@ -52,6 +81,10 @@
            return Optional.empty();
        }

+        if (isIdentityOrNarrowingProjection(project)) {


please create an issue about this on github

please modify assert framework for rules to check if rule is unable to be applied two times in a row

Did you mean the issue for check? Teradata#499

Or the issue for old ProjectionPushDown optimizer? Do we really want to fix that?

kokosing · 2017-02-03T07:32:31Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughExchange.java

+        return Optional.of(result);
+    }
+
+    private boolean isIdentityOrNarrowingProjection(ProjectNode project)


use project.isIdentity() here

what about shuffle projection?

Yeah - I think we should not do anything about shuffling projection. What is the point?
And actually this method returns true for shulffling projection.

I renamed method to isSymbolToSymbolProjection

kokosing · 2017-02-03T07:33:38Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughExchange.java

+
+            Assignments.Builder newAssignments = Assignments.builder();
+            for (Symbol symbol : project.getOutputSymbols()) {
+                newAssignments.put(symbol, symbol.toSymbolReference());


use Assignemnts.ideintity()

losipiuk · 2017-02-03T16:16:55Z

I addressed the comments. Some questions.

kokosing

some minor comments

kokosing · 2017-02-06T08:02:31Z

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/SymbolAliases.java

@@ -81,6 +82,13 @@ public SymbolReference get(String alias)
        return result;


use getOptional in implementation of get

kokosing · 2017-02-06T08:03:29Z

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/PlanMatchPattern.java

+        return result;
+    }
+
+    public static PlanMatchPattern values(List<String> aliases)


maybe add also a method which accepts var-args

kokosing · 2017-02-06T08:04:04Z

presto-main/src/test/java/com/facebook/presto/sql/planner/iterative/rule/test/PlanBuilder.java

@@ -124,7 +126,7 @@ public ExchangeBuilder addSource(PlanNode source)
            return this;
        }

-        public ExchangeBuilder addInputsSet(Symbol ... inputs)
+        public ExchangeBuilder addInputsSet(Symbol... inputs)


so why do I still see this?

kokosing · 2017-02-06T08:05:36Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughExchange.java

+ *  </pre>
+ *
+ *
+ *  To avoid looping this optimizer will not be fired if upper Project contains just symbol references.


have you posted an issue on github about this problem in legacy rule? If so can you please attach the link to it?

Discussed that it is about need for projection above Exchange node sometimes.

This is the issue: #7307

losipiuk

Updated

losipiuk · 2017-02-06T12:19:37Z

presto-main/src/test/java/com/facebook/presto/sql/planner/iterative/rule/test/PlanBuilder.java

@@ -124,7 +126,7 @@ public ExchangeBuilder addSource(PlanNode source)
            return this;
        }

-        public ExchangeBuilder addInputsSet(Symbol ... inputs)
+        public ExchangeBuilder addInputsSet(Symbol... inputs)


Weird. Annotate in IntelliJ showed last modification from older commit. Fixed.

losipiuk · 2017-02-06T12:28:39Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughExchange.java

+ *  </pre>
+ *
+ *
+ *  To avoid looping this optimizer will not be fired if upper Project contains just symbol references.


Discussed that it is about need for projection above Exchange node sometimes.

This is the issue: #7307

findepi · 2017-02-11T15:25:10Z

presto-main/src/test/java/com/facebook/presto/sql/planner/iterative/rule/test/PlanBuilder.java

@@ -78,7 +78,7 @@ public Symbol symbol(String name, Type type)
    {
        Symbol symbol = new Symbol(name);

-        Type old = symbols.get(symbol);
+        Type old = symbols.put(symbol, type);


Either retain get here or remove put 5 lines down the road.

(minor)

erichwang · 2017-03-08T03:00:47Z

Just a heads up: I already reviewed and pushed the commit "Extend support for exchange in PlanMatchingVisitor".

martint

Minor comments, but otherwise looks good. Please reassign to me once you've addressed them and I'll merge it.

martint · 2017-05-15T20:36:26Z

...a/com/facebook/presto/sql/planner/iterative/rule/test/TestPushProjectionThroughExchange.java

@@ -0,0 +1,156 @@
+/*


Squash this commit with the one that ports the rule.

Sure.

But note that test fails without the "changes" commit, as just-ported code does not ensure that Node after rewrite exposes same number of outputs.

Maybe we should squash all the three commits together. I left them separate mostly to make review easier.

martint · 2017-05-15T20:37:19Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughExchange.java

@@ -0,0 +1,131 @@
+/*


Rename this commit to "Migrate projection pushdown through exchange to iterative rule"

martint · 2017-05-15T20:38:07Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughExchange.java

@@ -35,6 +35,37 @@
 import java.util.Map;


Instead of repeating the commit title of the other commit with the "(changes)" qualifier, describe what the changes are.

Changed commit message to:

Ensure output symbols do not change after pushing projection through exchange

losipiuk

Addressed and rebased to origin/master.

As I mentioned in one of the replies I would consider merging the Ensure output symbols do not change after pushing projection through exchange into Migrate projection pushdown through exchange to iterative rule

losipiuk · 2017-05-16T15:27:00Z

...a/com/facebook/presto/sql/planner/iterative/rule/test/TestPushProjectionThroughExchange.java

@@ -0,0 +1,156 @@
+/*


Sure.

But note that test fails without the "changes" commit, as just-ported code does not ensure that Node after rewrite exposes same number of outputs.

Maybe we should squash all the three commits together. I left them separate mostly to make review easier.

losipiuk · 2017-05-16T15:44:55Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughExchange.java

@@ -35,6 +35,37 @@
 import java.util.Map;


Changed commit message to:

Ensure output symbols do not change after pushing projection through exchange

Beside porting the code changes to rule where needto to ensure output symbols do not change after pushing projection through exchange.

losipiuk requested a review from kokosing January 31, 2017 17:39

facebook-github-bot added the CLA Signed label Jan 31, 2017

kokosing suggested changes Feb 1, 2017

View reviewed changes

martint added the planner label Feb 2, 2017

losipiuk force-pushed the lo/projection-pushdown-new-optimizer branch 2 times, most recently from 6a609fc to bafc65c Compare February 2, 2017 15:32

losipiuk force-pushed the lo/projection-pushdown-new-optimizer branch from bafc65c to 76872e2 Compare February 2, 2017 16:38

findepi mentioned this pull request Feb 2, 2017

Add RemoveEmptyDelete rule #7263

Closed

kokosing suggested changes Feb 3, 2017

View reviewed changes

losipiuk force-pushed the lo/projection-pushdown-new-optimizer branch from 76872e2 to de1ac09 Compare February 3, 2017 16:14

losipiuk force-pushed the lo/projection-pushdown-new-optimizer branch 2 times, most recently from 1f823e2 to 70baada Compare February 3, 2017 16:18

kokosing approved these changes Feb 6, 2017

View reviewed changes

losipiuk force-pushed the lo/projection-pushdown-new-optimizer branch from 70baada to 81458c6 Compare February 6, 2017 12:29

losipiuk commented Feb 6, 2017

View reviewed changes

findepi previously requested changes Feb 11, 2017

View reviewed changes

losipiuk force-pushed the lo/projection-pushdown-new-optimizer branch from 81458c6 to 8f99fdc Compare February 23, 2017 09:32

losipiuk force-pushed the lo/projection-pushdown-new-optimizer branch from 8f99fdc to 31523c3 Compare March 20, 2017 14:00

losipiuk force-pushed the lo/projection-pushdown-new-optimizer branch from 31523c3 to 1f6588a Compare April 10, 2017 16:39

dain assigned martint Apr 17, 2017

martint suggested changes May 15, 2017

View reviewed changes

martint assigned losipiuk and unassigned martint May 15, 2017

losipiuk force-pushed the lo/projection-pushdown-new-optimizer branch 2 times, most recently from 982dac3 to 84959ec Compare May 16, 2017 15:52

losipiuk commented May 16, 2017

View reviewed changes

losipiuk assigned martint and unassigned losipiuk May 16, 2017

losipiuk added 8 commits May 17, 2017 10:37

Register new symbols in map in PlanBuilder

570f5e6

Add PlanMatchPattern.with(String alias) method

f2be652

Add varargs versions of PlanMatchPattern.with* methods

39655c9

Add values() matcher with list of aliases

10ae498

Add union support to optimizer tests PlanBuilder

c64b852

Migrate projection pushdown through union to iterative rule

2bd8440

Migrate projection pushdown through exchange to iterative rule

7bd4967

Beside porting the code changes to rule where needto to ensure output symbols do not change after pushing projection through exchange.

Use PushProjectionThrough{Union,Exchange} optimizers

cdd5b68

losipiuk force-pushed the lo/projection-pushdown-new-optimizer branch from 84959ec to cdd5b68 Compare May 17, 2017 08:41

martint merged commit 648528b into prestodb:master May 17, 2017

losipiuk deleted the lo/projection-pushdown-new-optimizer branch May 18, 2017 12:52

		@@ -81,6 +82,13 @@ public SymbolReference get(String alias)
		return result;

Migrate ProjectionPushDown to iterative optimizer #7256

Migrate ProjectionPushDown to iterative optimizer #7256

Conversation

losipiuk commented Jan 31, 2017

losipiuk commented Jan 31, 2017

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk commented Feb 2, 2017

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk commented Feb 3, 2017

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichwang commented Mar 8, 2017

martint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment