Implement dynamic partition pruning #1072

raunaqmorarka · 2019-07-02T20:06:01Z

Introduce collecting DynamicFilterSummary on build side of Join

Introduce DynamicFilterSummary
Collect DynamicFilterSummary before HashBuilder

Expose DynamicFilterResource endpoint on coordinator for collecting DynamicFilterSummaries from worker nodes. Feed coordinator with DynamicFilterSummaries from worker nodes

Add DynamicFilterResource
Add InMemoryDynamicFilterClient

Use DynamicFilterSummaries collected on coordinator to filter out splits in SplitManager

Register tasks in DynamicFilterService
Add DynamicFilterDescription
Pass Future with DynamicFilterDescription to the SplitManager
Add dynamic partition pruning to Hive connector

sopel39 · 2019-07-03T07:27:19Z

@rzeyde-varada could you also assist reviewing it?

sopel39 · 2019-07-03T07:28:12Z

test failures seems relevant to this feature

raunaqmorarka · 2019-07-03T20:38:53Z

test failures seems relevant to this feature

I've resolved the test failures now

rzeyde-varada

Reviewed most of the code - looks good overall, a few questions/suggestions inside.
Will continue the review tomorrow.

presto-main/src/main/java/io/prestosql/sql/analyzer/FeaturesConfig.java

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

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveSplitSource.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveSplitSource.java

presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java

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

presto-main/src/main/java/io/prestosql/execution/SqlStageExecution.java

presto-main/src/main/java/io/prestosql/operator/DynamicFilterClient.java

presto-main/src/main/java/io/prestosql/operator/DynamicFilterClientFactory.java

presto-main/src/main/java/io/prestosql/operator/DynamicFilterClientSupplier.java

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

presto-main/src/main/java/io/prestosql/server/DynamicFilterResource.java

presto-main/src/main/java/io/prestosql/server/ServerMainModule.java

presto-main/src/main/java/io/prestosql/server/DynamicFilterResource.java

presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java

rzeyde-varada

Finished the review - many thanks for the contribution!

A small question about integration tests:
Would it be possible to adapt MemorySplitManager, so we can run a smoke integration test for dynamic partition pruning?

presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java

presto-main/src/main/java/io/prestosql/server/DynamicFilterResource.java

presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java

presto-main/src/main/java/io/prestosql/operator/DynamicFilterSourceOperator.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveSplitSource.java

presto-main/src/main/java/io/prestosql/operator/DynamicFilterSourceOperator.java

raunaqmorarka · 2019-07-15T22:29:56Z

Finished the review - many thanks for the contribution!

A small question about integration tests:
Would it be possible to adapt MemorySplitManager, so we can run a smoke integration test for dynamic partition pruning?

Thanks for the thorough review :)
It doesn't look like there is an equivalent of hive partitions in memory connector.
@sopel39 Is it possible to test partition pruning through memory connector ? Also, would it be useful to add product tests with partitioned hive tables for this change ?

martint

Some initial comments. I'm looking at the rest of the code now.

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveSplitSource.java

presto-spi/src/main/java/io/prestosql/spi/DynamicFilterDescription.java

presto-main/src/main/java/io/prestosql/operator/DynamicFilterSummary.java

presto-main/src/main/java/io/prestosql/server/DynamicFilterResource.java

sopel39

tests left to review

sopel39 · 2020-06-04T09:57:05Z

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

@@ -54,22 +57,25 @@
    // Mapping from dynamic filter ID to its build channel indices.
    private final Map<String, Integer> buildChannels;

-    private final TypeProvider types;
+    // Mapping from dynamic filter ID to its build channel type.
+    private final Map<String, Type> filterBuildTypes;


nit: we probably should have dedicated class for filter Id at this point instead of string

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

sopel39 · 2020-06-04T10:07:14Z

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

+        return Futures.transform(resultFuture, this::convertTupleDomain, directExecutor());
+    }
+
+    public ListenableFuture<Map<Symbol, Domain>> getNodeLocalDynamicFilterForSymbols()


nit: Local specific logic should be moved out of LocalDynamicFilter eventually

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

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

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

sopel39

LGTM % test improvement

presto-main/src/test/java/io/prestosql/server/TestDynamicFilterService.java

presto-main/src/test/java/io/prestosql/sql/planner/TestLocalDynamicFilterCollector.java

presto-main/src/test/java/io/prestosql/sql/planner/TestLocalDynamicFilter.java

presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java

presto-main/src/test/java/io/prestosql/server/TestDynamicFilterService.java

sopel39 · 2020-06-07T21:47:13Z

fixes: #52

presto-main/src/test/java/io/prestosql/server/TestDynamicFilterService.java

sopel39 · 2020-06-09T09:16:46Z

benchmark results with partitions (significant CPU and duration improvement)
dynamic_filtering_partitions.pdf

benchmark results without partitions:
dynamic_filtering_no_partitions.pdf
There is some CPU improvement visible, but it's bogus due to benchmark instability for baseline (I'm reruning it now).

martint · 2020-06-09T15:35:55Z

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

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveSplitSource.java

presto-main/src/main/java/io/prestosql/execution/SqlQueryExecution.java

dain · 2020-06-09T18:55:48Z

presto-main/src/main/java/io/prestosql/execution/StageState.java

@@ -88,4 +88,29 @@ public boolean isFailure()
    {
        return failureState;
    }
+
+    public boolean noMoreTasks()


This should be named something else. noMoreSplits sounds like a command, and this simply interrogates the current state.

presto-main/src/main/java/io/prestosql/execution/StageState.java

dain · 2020-06-09T18:57:02Z

presto-main/src/main/java/io/prestosql/execution/StageState.java

+            case RUNNING:
+            case FINISHED:
+            case CANCELED:
+                // no more workers will be added to the query


The comments don't seem to match the code... this doesn't change anything

This code was extracted as-is from io.prestosql.execution.scheduler.SqlQueryScheduler.StageLinkage#processScheduleResults
#1072 (comment)
Are we guaranteed that there will be no more tasks added (e.g when new workers come with co-located joins) when we reach SCHEDULING_SPLITS state ?
If not, we still need some way to figure out in DynamicFilterService when it's safe to assume that no new tasks will be added which can generate more values from build-side of join and we can use the dynamic filters reported from completion of existing build-side operators for filtering on the probe side.

@dain

Looking at SourcePartitionedScheduler#finalizeTaskCreationIfNecessary it will splitPlacementPolicy.lockDownNodes(); and set state to SCHEDULING_SPLIT. It seems that at this point no more nodes will participate in query.

However, can stage state can change to RUNNING via SqlStageExecution#schedulingComplete() and skip splitPlacementPolicy.lockDownNodes(); call?

I guess SqlQueryScheduler.StageLinkage#processScheduleResults would also break then

I was referring to the tone of the comments, not the contents. The comments imply that a change will occur. For example, the comment below says DO NOT complete a FAILED or ABORTED stage, but this is not a command. The code where this was extracted was making changes to the stage, so the comments were appropriate.

comments applied

sopel39 · 2020-06-10T19:50:20Z

merged, thanks!

bitsondatadev · 2020-08-22T00:31:28Z

@simpligility I think this would be a great PR to showcase for the Presto Twitch show. @raunaqmorarka any objections to us showcasing your work? Further would you be interested in joining the show?

cla-bot bot added the cla-signed label Jul 2, 2019

raunaqmorarka mentioned this pull request Jul 2, 2019

Dynamic filtering #52

Closed

raunaqmorarka force-pushed the runtime-partition-pruning branch 4 times, most recently from 7818da6 to 79d9a73 Compare July 3, 2019 17:35

rzeyde-varada reviewed Jul 10, 2019

View reviewed changes

rzeyde-varada reviewed Jul 11, 2019

View reviewed changes

presto-main/src/main/java/io/prestosql/operator/DynamicFilterSourceOperator.java Outdated Show resolved Hide resolved

rzeyde-varada mentioned this pull request Jul 11, 2019

Implement local dynamic filtering for broadcast inner-joins #931

Closed

raunaqmorarka force-pushed the runtime-partition-pruning branch 4 times, most recently from 567039d to 82e16ba Compare July 15, 2019 22:10

raunaqmorarka force-pushed the runtime-partition-pruning branch from 52fee8e to 678a943 Compare August 22, 2019 10:41

raunaqmorarka force-pushed the runtime-partition-pruning branch from 678a943 to 9ee27c9 Compare September 13, 2019 18:25

raunaqmorarka force-pushed the runtime-partition-pruning branch 3 times, most recently from f042881 to 715227e Compare October 8, 2019 22:02

raunaqmorarka force-pushed the runtime-partition-pruning branch 3 times, most recently from 058f13b to b760b51 Compare October 10, 2019 20:56

martint self-requested a review October 16, 2019 15:29

martint reviewed Oct 16, 2019

View reviewed changes

martint previously requested changes Oct 17, 2019

View reviewed changes

presto-main/src/main/java/io/prestosql/server/DynamicFilterResource.java Outdated Show resolved Hide resolved

raunaqmorarka force-pushed the runtime-partition-pruning branch from 1917330 to cd7f2de Compare June 3, 2020 18:53

sopel39 reviewed Jun 4, 2020

View reviewed changes

presto-main/src/main/java/io/prestosql/sql/planner/LocalDynamicFilter.java Outdated Show resolved Hide resolved

presto-main/src/main/java/io/prestosql/sql/planner/LocalDynamicFilter.java Outdated Show resolved Hide resolved

raunaqmorarka force-pushed the runtime-partition-pruning branch from cd7f2de to 7339097 Compare June 4, 2020 11:33

raunaqmorarka requested a review from sopel39 June 4, 2020 11:43

sopel39 reviewed Jun 5, 2020

View reviewed changes

raunaqmorarka force-pushed the runtime-partition-pruning branch 2 times, most recently from 19be880 to 2f13dfb Compare June 8, 2020 11:36

raunaqmorarka requested a review from sopel39 June 8, 2020 11:38

sopel39 approved these changes Jun 9, 2020

View reviewed changes

presto-main/src/test/java/io/prestosql/server/TestDynamicFilterService.java Outdated Show resolved Hide resolved

sopel39 mentioned this pull request Jun 9, 2020

Pass dynamic filters back to workers from coordinator #3972

Closed

raunaqmorarka force-pushed the runtime-partition-pruning branch 2 times, most recently from 2f0f83f to 796c841 Compare June 9, 2020 14:00

sopel39 reviewed Jun 9, 2020

View reviewed changes

presto-main/src/main/java/io/prestosql/sql/planner/DistributedExecutionPlanner.java Outdated Show resolved Hide resolved

dain reviewed Jun 9, 2020

View reviewed changes

raunaqmorarka added 3 commits June 10, 2020 11:37

Remove dynamic filters with expression which are not a SymbolReference

9fe77b6

Implement runtime partition pruning

ac45ad2

Rename LocalDynamicFilter to LocalDynamicFilterConsumer

dc2eedb

raunaqmorarka force-pushed the runtime-partition-pruning branch from 796c841 to dc2eedb Compare June 10, 2020 06:08

raunaqmorarka requested a review from dain June 10, 2020 06:35

sopel39 merged commit 7f278db into trinodb:master Jun 10, 2020

sopel39 mentioned this pull request Jun 10, 2020

Release notes for 335 #3886

Closed

9 tasks

raunaqmorarka deleted the runtime-partition-pruning branch June 11, 2020 03:27

raunaqmorarka mentioned this pull request Nov 4, 2021

Perform one more dynamic partition pruning check #9866

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement dynamic partition pruning #1072

Implement dynamic partition pruning #1072

raunaqmorarka commented Jul 2, 2019 •

edited

Loading

sopel39 commented Jul 3, 2019

sopel39 commented Jul 3, 2019

raunaqmorarka commented Jul 3, 2019

rzeyde-varada left a comment

rzeyde-varada left a comment •

edited

Loading

raunaqmorarka commented Jul 15, 2019

martint left a comment

sopel39 left a comment

sopel39 Jun 4, 2020

sopel39 Jun 4, 2020

sopel39 left a comment

sopel39 commented Jun 7, 2020

sopel39 commented Jun 9, 2020

martint commented Jun 9, 2020

dain Jun 9, 2020

dain Jun 9, 2020

raunaqmorarka Jun 10, 2020

sopel39 Jun 10, 2020

dain Jun 10, 2020

sopel39 commented Jun 10, 2020

bitsondatadev commented Aug 22, 2020

Implement dynamic partition pruning #1072

Implement dynamic partition pruning #1072

Conversation

raunaqmorarka commented Jul 2, 2019 • edited Loading

sopel39 commented Jul 3, 2019

sopel39 commented Jul 3, 2019

raunaqmorarka commented Jul 3, 2019

rzeyde-varada left a comment

Choose a reason for hiding this comment

rzeyde-varada left a comment • edited Loading

Choose a reason for hiding this comment

raunaqmorarka commented Jul 15, 2019

martint left a comment

Choose a reason for hiding this comment

sopel39 left a comment

Choose a reason for hiding this comment

sopel39 Jun 4, 2020

Choose a reason for hiding this comment

sopel39 Jun 4, 2020

Choose a reason for hiding this comment

sopel39 left a comment

Choose a reason for hiding this comment

sopel39 commented Jun 7, 2020

sopel39 commented Jun 9, 2020

martint commented Jun 9, 2020

dain Jun 9, 2020

Choose a reason for hiding this comment

dain Jun 9, 2020

Choose a reason for hiding this comment

raunaqmorarka Jun 10, 2020

Choose a reason for hiding this comment

sopel39 Jun 10, 2020

Choose a reason for hiding this comment

dain Jun 10, 2020

Choose a reason for hiding this comment

sopel39 commented Jun 10, 2020

bitsondatadev commented Aug 22, 2020

raunaqmorarka commented Jul 2, 2019 •

edited

Loading

rzeyde-varada left a comment •

edited

Loading