SPI and optimizer rule for connectors that can support complete topN … #4249

Parth-Brahmbhatt · 2020-06-26T21:35:49Z

…pushdown

Parth-Brahmbhatt · 2020-06-26T21:38:46Z

I have made the assumption that a connector that states it can support complete topN will guarantee N or less result, if we think that is too strict a requirement we can do something similar to limit pushdown.

presto-main/src/main/java/io/prestosql/metadata/Metadata.java

presto-spi/src/main/java/io/prestosql/spi/connector/TopNFunction.java

presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorMetadata.java

presto-main/src/test/java/io/prestosql/connector/MockConnectorFactory.java

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushTopNIntoTableScan.java

martint · 2020-07-17T20:12:21Z

presto-spi/src/main/java/io/prestosql/spi/connector/TopN.java

+
+import static java.util.Objects.requireNonNull;
+
+public class TopN


Having a dedicated class for this seems overkill. Every place that uses this has immediate access to the count and SortItems from a TopNNode. The SPI method can just take both fields separately.

Removed the class and added topNCount and sortItems as just arguments.

martint · 2020-07-29T00:27:45Z

presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorMetadata.java

+     * If the connector can handle TopN Pushdown it should return a new table handle which will replace the existing
+     * table handle in the TableScan.
+     */
+    default Optional<ConnectorTableHandle> applyTopN(


I think we'll want to return a structure similar to applyLimit, where it indicates whether the limit is guaranteed by the connector. In the case of a distributed connector implementation, it may not be able to perform a global top N, but it could still benefit from pushing down the operation, for instance, to each individual storage shard.

Added TopNApplicationResult which is basically same as LimitApplicationResult for now and added a test case that tests that when limitGuaranteed is set to false we create a plan with TopNPartial stage with updated connector table handle reference.

@martint can you review this again when you find some time?

martint · 2020-08-03T23:24:52Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushTopNIntoTableScan.java

-                                tableScan.getOutputSymbols(),
-                                tableScan.getAssignments()));
+                    if (!result.isTopNGuaranteed()) {
+                        node = new TopNNode(topNNode.getId(), node, topNNode.getCount(), topNNode.getOrderingScheme(), TopNNode.Step.PARTIAL);


I believe this should be Step.FINAL. The partial Top N is being handled by the connector at this point.

Followed up here: #4249 (comment)

findepi · 2021-02-23T11:52:56Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushTopNIntoTableScan.java

+    private static final Pattern<TopNNode> PATTERN = topN().with(source().matching(
+            tableScan().capturedAs(TABLE_SCAN)));


I think this should trigger no SINGLE step only.
We should not push partial and final separately (and IMO -- we should not push them at all)

findepi · 2021-02-23T12:00:24Z

presto-main/src/main/java/io/prestosql/sql/planner/PlanOptimizers.java

@@ -623,7 +624,8 @@ public PlanOptimizers(
                        new CreatePartialTopN(),
                        new PushTopNThroughProject(),
                        new PushTopNThroughOuterJoin(),
-                        new PushTopNThroughUnion())));
+                        new PushTopNThroughUnion(),
+                        new PushTopNIntoTableScan(metadata))));


This should be added to pushIntoTableScanOptimizer, when other PushXxxIntoTableScan rules are registered.

This is important for two reasons:

this will unlock other pushdowns once TopN is pushed into TableScan fully

this will let the rule operate on TopNNode while it is SINGLE step (and consume it fully), making reasoning about engine-connector interactions simpler

@wendigo is going to address this in #6847

findepi · 2021-02-23T12:03:55Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushTopNIntoTableScan.java

+                            tableScan.getAssignments());
+
+                    if (!result.isTopNGuaranteed()) {
+                        node = new TopNNode(topNNode.getId(), node, topNNode.getCount(), topNNode.getOrderingScheme(), TopNNode.Step.FINAL);


Relates to #4249 (comment)

Inserting FINAL step is not correct when the rule triggered on TopNNode with Step PARTIAL.

I suggest

make the rule trigger for SINGLE step only (my preferred)

i am aware sibling PushLimitIntoTableScan triggers for partial limits, but i am not convinced it is beneficial

keep same step as it used to be used (basically, use topNNode.replaceChildren)

(this is not being addressed in @wendigo #6847)

findepi · 2021-02-23T12:14:45Z

cc @losipiuk

cla-bot bot added the cla-signed label Jun 26, 2020

martint self-requested a review June 26, 2020 21:38

Parth-Brahmbhatt force-pushed the topn-pushdown branch from 87f81a0 to eb2f41c Compare June 26, 2020 21:39

Parth-Brahmbhatt requested a review from findepi June 26, 2020 21:52

Parth-Brahmbhatt force-pushed the topn-pushdown branch 2 times, most recently from 9ec82c0 to 807e383 Compare June 29, 2020 16:51

findepi reviewed Jul 1, 2020

View reviewed changes

martint reviewed Jul 17, 2020

View reviewed changes

Parth-Brahmbhatt force-pushed the topn-pushdown branch from c845a4e to 27a14f2 Compare July 23, 2020 18:09

martint reviewed Jul 29, 2020

View reviewed changes

martint self-requested a review August 3, 2020 19:00

martint reviewed Aug 3, 2020

View reviewed changes

martint self-requested a review August 4, 2020 22:42

martint force-pushed the topn-pushdown branch from 215b33d to 439ce77 Compare August 5, 2020 03:39

Add support for TopN pushdown

9c94323

martint force-pushed the topn-pushdown branch from 439ce77 to 9c94323 Compare August 5, 2020 03:40

martint merged commit 3602068 into trinodb:master Aug 5, 2020

martint added this to the 340 milestone Aug 5, 2020

This was referenced Aug 5, 2020

Release notes for 340 #4527

Closed

Allow connectors to participate in query optimization #18

Open

findepi mentioned this pull request Feb 23, 2021

Remember best known statistics for TableHandle on engine side #6998

Closed

findepi reviewed Feb 23, 2021

View reviewed changes

findepi mentioned this pull request Feb 25, 2021

Add support for TopN pushdown in JDBC connectors #6847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPI and optimizer rule for connectors that can support complete topN … #4249

SPI and optimizer rule for connectors that can support complete topN … #4249

Parth-Brahmbhatt commented Jun 26, 2020

Parth-Brahmbhatt commented Jun 26, 2020

martint Jul 17, 2020

Parth-Brahmbhatt Jul 23, 2020

martint Jul 29, 2020

Parth-Brahmbhatt Jul 29, 2020

Parth-Brahmbhatt Aug 3, 2020

martint Aug 3, 2020

Parth-Brahmbhatt Aug 4, 2020

findepi Feb 23, 2021

findepi Feb 23, 2021

findepi Feb 23, 2021

findepi Feb 23, 2021

findepi commented Feb 23, 2021


		import static java.util.Objects.requireNonNull;

		public class TopN

		private static final Pattern<TopNNode> PATTERN = topN().with(source().matching(
		tableScan().capturedAs(TABLE_SCAN)));

SPI and optimizer rule for connectors that can support complete topN … #4249

SPI and optimizer rule for connectors that can support complete topN … #4249

Conversation

Parth-Brahmbhatt commented Jun 26, 2020

Parth-Brahmbhatt commented Jun 26, 2020

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

findepi commented Feb 23, 2021