Iterative optimizer #6956

martint · 2016-12-23T02:20:59Z

This optimizer decouples the traversal of the plan tree (IterativeOptimizer)
from the transformation logic (Rule). The optimization loop applies rules
recursively until a fixpoint is reached.

It's implemented as PlanOptimizer so that it fits right into the existing
framework.

Extracted from #6700

This makes it easier to extend the node hierarchy for tests in the upcoming iterative optimizer.

kokosing

What about these comments:
13e6b6a#r93578038
13e6b6a#r93575465
13e6b6a#r93576086
13e6b6a#r93576974

kokosing · 2016-12-23T06:07:47Z

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/AssignUniqueId.java

@@ -72,4 +74,11 @@ public Symbol getIdColumn()
    {
        return visitor.visitAssignUniqueId(this, context);
    }
+
+    @Override
+    public PlanNode replaceChildren(List<PlanNode> newChildren)


what about renaming this to replaceSources? You have getSources() but replaceChildren, it sounds like they were referencing to two different things.

"sources" is a legacy term. It used to be accurate a long time ago, before we even had support for "index joins". Then, with that feature, and now with Apply, it can mean either source or subplan. So "child" is more correct now.

kokosing · 2016-12-23T06:19:49Z

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

+        return node(idAllocator.getNextId(), source);
+    }
+
+    private GenericNode node(PlanNodeId id)


I think you could have only two node methods:

node(PlanNodeId id, PlanNode ... sources); node(PlanNode ... sources);

Ah, duh! I renamed the old filter/project/join/values methods and forgot to deal with the duplication :)

kokosing · 2016-12-23T06:23:30Z

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

+        int yGroup = getChildGroup(memo, memo.getRootGroup());
+        int zGroup = getChildGroup(memo, yGroup);
+
+        PlanNode rewrittenW = memo.getNode(zGroup).getSources().get(0);


it is just w. There is no rewrite here.

For this node instance in particular, sure. Potentially, it's a rewritten node, because that's just what the memo does with every node in the plan tree in order to replace their children with group references.

In this case, I'm trying to simulate what a rule would see. It would not be correct to use w directly.

kokosing · 2016-12-23T06:26:10Z

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

+
+        Memo memo = new Memo(idAllocator, x);
+
+        assertEquals(memo.getGroupCount(), 4);


why not to use IterativeOptimizer here with some simple lambda rule? Then you could also check the traversal of IterativeOptimizer

Something like:

optimizer(node -> if (node == y || node == z) return node(node.getSource())).optimize(x)

I could do that, but I wanted to test the Memo in isolation.

kokosing · 2016-12-23T06:29:34Z

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

+
+import static org.testng.Assert.assertEquals;
+
+public class TestMemo


can you add a test for cycle? Rules can have bugs, so it would be nice to see how memo handles that.

I'm not sure how a rule would be able to cause a cycle. Unless rules make up group ids out of thin air, I don't think it's possible to do so. Let me think about it.

kokosing · 2016-12-23T06:30:51Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

+    @Config("experimental.iterative-optimizer-enabled")
+    public FeaturesConfig setIterativeOptimizerEnabled(boolean value)
+    {
+        this.newOptimizerEnabled = value;


rename the field

martint · 2016-12-23T07:48:42Z

What about these comments:
13e6b6a#r93578038

It's enabled for all tests deriving from AbstractTestQueries. I didn't want to add another instance to avoid increasing build times.

13e6b6a#r93575465

I expect Lookup to evolve beyond just exposing stuff from Memo (e.g., to provide other things such as "traits" -- effective predicate, sortedness, cost, etc.) that doesn't belong in the Memo structure. We can always fold it into it if necessary.

13e6b6a#r93576086

Let me play with it. It might be over-engineering it.

13e6b6a#r93576974

I'll take a look. The exception may be a remnant from some intermediate code I was working on. Regardless, at some point we need to get rid of OutputNode. It serves no purpose other than a glorified ProjectNode.

kokosing · 2016-12-23T07:54:38Z

It's enabled for all tests deriving from AbstractTestQueries. I didn't want to add another instance to avoid increasing build times.

What about testing the other way round? Iterative optimizer is disabled by default which could hide some regressions in case that iterative optimizer "fixes" plan in case of bugs in legacy rules.

kokosing

Just one comment with testing cycle in Memo and IMO it is ready to go.

martint · 2016-12-23T08:03:42Z

What about testing the other way round? Iterative optimizer is disabled by default which could hide some regressions in case that iterative optimizer "fixes" plan in case of bugs in legacy rules.

Turn on the new optimizer in prod, then? ;)

In all seriousness, this is a challenge. Until we come up with a better story around how to deal with the matrix of features vs number of tests (i.e., #6910), we're going to have to live with compromises like this.

kokosing · 2016-12-23T08:17:53Z

Turn on the new optimizer in prod, then? ;)

Sounds good to me. ;)

In all seriousness, this is a challenge. Until we come up with a better story around how to deal with the matrix of features vs number of tests (i.e., #6910), we're going to have to live with compromises like this.

Lets put #6910 to higher priority then, I will talk about it within TD.

kokosing · 2016-12-23T08:24:22Z

What about common subplan extraction? Bugs there could cause cycles. But ok, let's postpone this test until such rewrite (common subplan extraction) got possible. pt., 23.12.2016, 09:07 użytkownik Martin Traverso <notifications@github.com> napisał:

…

***@***.**** commented on this pull request. ------------------------------ In presto-main/src/test/java/com/facebook/presto/sql/planner/iterative/TestMemo.java <#6956>: > + * limitations under the License. + */ +package com.facebook.presto.sql.planner.iterative; + +import com.facebook.presto.sql.planner.PlanNodeIdAllocator; +import com.facebook.presto.sql.planner.Symbol; +import com.facebook.presto.sql.planner.plan.PlanNode; +import com.facebook.presto.sql.planner.plan.PlanNodeId; +import com.google.common.collect.ImmutableList; +import org.testng.annotations.Test; + +import java.util.List; + +import static org.testng.Assert.assertEquals; + +public class TestMemo I'm not sure how a rule would be able to cause a cycle. Unless rules are making up group ids out of thin air, I don't think it's possible to do so. Let me think about it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6956>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHN_-yIB81ksy7iiQ88tCfY3luLY9do4ks5rK4EzgaJpZM4LUh_M> .

dain · 2016-12-23T17:49:44Z

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/MetadataDeleteNode.java

+    @Override
+    public PlanNode replaceChildren(List<PlanNode> newChildren)
+    {
+        return new MetadataDeleteNode(getId(), target, output, tableLayout);


assert newChildren.isEmpty()?

This optimizer decouples the traversal of the plan tree (IterativeOptimizer) from the transformation logic (Rule). The optimization loop applies rules recursively until a fixpoint is reached. It's implemented as PlanOptimizer so that it fits right into the existing framework.

martint force-pushed the iterative branch 2 times, most recently from 7db2d56 to d7b54c2 Compare December 23, 2016 02:26

Move replaceChild logic to per-node method

64004e8

This makes it easier to extend the node hierarchy for tests in the upcoming iterative optimizer.

martint force-pushed the iterative branch 2 times, most recently from eea7ab9 to 1d57f9d Compare December 23, 2016 04:57

kokosing suggested changes Dec 23, 2016

View reviewed changes

kokosing approved these changes Dec 23, 2016

View reviewed changes

martint force-pushed the iterative branch from 1d57f9d to 41931a6 Compare December 23, 2016 08:09

dain approved these changes Dec 23, 2016

View reviewed changes

martint force-pushed the iterative branch from 41931a6 to aafcb1e Compare December 28, 2016 18:22

martint self-assigned this Dec 28, 2016

martint added the accepted label Dec 28, 2016

facebook-github-bot added the CLA Signed label Dec 29, 2016

martint merged commit addf493 into prestodb:master Jan 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iterative optimizer #6956

Iterative optimizer #6956

martint commented Dec 23, 2016

kokosing left a comment

kokosing Dec 23, 2016

martint Dec 23, 2016

kokosing Dec 23, 2016

martint Dec 23, 2016

kokosing Dec 23, 2016

martint Dec 23, 2016

kokosing Dec 23, 2016

martint Dec 23, 2016

kokosing Dec 23, 2016

martint Dec 23, 2016 •

edited

kokosing Dec 23, 2016

martint commented Dec 23, 2016

kokosing commented Dec 23, 2016

kokosing left a comment

martint commented Dec 23, 2016

kokosing commented Dec 23, 2016

kokosing commented Dec 23, 2016 via email •

edited

dain Dec 23, 2016


		Memo memo = new Memo(idAllocator, x);

		assertEquals(memo.getGroupCount(), 4);


		import static org.testng.Assert.assertEquals;

		public class TestMemo

Iterative optimizer #6956

Iterative optimizer #6956

Conversation

martint commented Dec 23, 2016

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

martint Dec 23, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martint commented Dec 23, 2016

kokosing commented Dec 23, 2016

kokosing left a comment

Choose a reason for hiding this comment

martint commented Dec 23, 2016

kokosing commented Dec 23, 2016

kokosing commented Dec 23, 2016 via email • edited

Choose a reason for hiding this comment

martint Dec 23, 2016 •

edited

kokosing commented Dec 23, 2016 via email •

edited