enable dynamic filtering by default #2793

sopel39 · 2020-02-11T13:09:19Z

No description provided.

sopel39 · 2020-02-17T10:26:08Z

Benchmarks (please take a look at CPU more as duration has rather high variability due to cloud env):

Benchmarks comparison-dynamic_filtering.pdf

Some notes:

Queries that got considerable perf improvement: tpcds/q16, tpcds/q15, tpcds/q18, tpcds/q20, tpcds/q21, tpcds/q33, tpcds/q39_1, ... and more. Generated data is unstructured (non-partitioned, non-sorted) so there should be more improvements with structured data.
dynamic filtering predicates also carry non null information for symbol. Therefore some outer joins were converted to inner joins (tpcds/q05, tpcds/q93). This explains speedup improvements to q93. We could potentially pushdown dynamic filters to all join branches, so that more outer joins get converted to inner joins. Dynamic filters are removed at the end of planning from places where they shouldn't be (e.b build side).
Because of how plan optimizers work, some join symbols got unaliased differently (e.g right equi-join symbols got unaliased to left equi-join symbols). This isn't directly related to dynamic filters. PR proposal to make unaliasing more deterministic: Remove redundant remote exchanges due to missing unaliasing #2853
There might be slight CPU degradation for tpcds/q23_1, tpcds/23_2. This could be due to different unaliasing in multi-join (using probe side equi symbols as outputs instead of build side equi-symbols). In this case join outputs dictionary blocks which might cause some additional overhead when processed by downstream operators. Preserve unnested dictionary in DictionaryBlock#unnest #2854 is a proposal to optimize dictionary blocks slightly.

raunaqmorarka · 2020-02-19T19:59:05Z

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

@@ -405,7 +403,7 @@ public PlanOptimizers(
                new CheckSubqueryNodesAreRewritten(),
                new StatsRecordingPlanOptimizer(
                        optimizerStats,
-                        new PredicatePushDown(metadata, typeAnalyzer, false)),
+                        new StatsRecordingPlanOptimizer(optimizerStats, new PredicatePushDown(metadata, typeAnalyzer, false, false))),


Since we are using a new instance of StatsRecordingPlanOptimizer for the 3 places where PredicatePushDown rule is applied, it looks like this will reset the stats each time for this rule (Check OptimizerStatsRecorder#register). Is that ok ? I think we would want to know the combined time taken of PredicatePushDown rule in all the invocations or record them all separately.

I will report to same optimizerStats every time, so it accumulates stats for PredicatePushDown

martint

Just some minor comments.

martint · 2020-02-22T21:44:32Z

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

@@ -279,8 +279,6 @@ public PlanOptimizers(
                        .add(new RemoveTrivialFilters())


Typo in commit message: "Prediacate"

martint · 2020-02-22T21:48:18Z

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

@@ -407,7 +405,7 @@ public PlanOptimizers(
                new CheckSubqueryNodesAreRewritten(),
                new StatsRecordingPlanOptimizer(
                        optimizerStats,
-                        new PredicatePushDown(metadata, typeAnalyzer, false)),


Hmm... this is going to have to change once we move PredicatePushdown to the iterative optimizer, as it will be interleaved with all other rules.

Yup. I think in this case it might be relatively easy to remove this dependency, so that dynamic filters don't change existing plans. I just didn't want to dig into this particular issue

martint · 2020-02-22T21:50:11Z

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

@@ -2042,7 +2042,7 @@ private PhysicalOperation createLookupJoin(
            ImmutableList.Builder<OperatorFactory> factoriesBuilder = new ImmutableList.Builder<>();


Can you add some details to the commit message explaining the motivation? I..e, what about grouped execution makes it incompatible with dynamic filters?

martint · 2020-02-22T21:59:16Z

presto-main/src/main/java/io/prestosql/split/PageSourceManager.java

@@ -13,15 +13,13 @@
 */


Can you add some details to the commit message explaining the motivation?

martint · 2020-02-22T21:59:49Z

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

@@ -117,7 +117,7 @@ public ConnectorPageSource createPageSource(ConnectorTransactionHandle transacti
                hiveSplit.getFileSize(),


Same here, can you add a comment in the commit message explaining the motivation?

Predicate pushdown pushes dynamic filters down the plan. At the end of planning, dynamic filters are removed from places where they are not valid (e.g build side, nested predicates). Because of this reorder joins should operate on plan that doesn't have dynamic filters as it might cause stats and cost misestimates.

When isEnableDynamicFiltering is disabled, then dynamic filters won't be added by planner.

Dynamic filtering assumes that join build side is evaulated fixed (task_concurrency) amount of times. This constraint is not satisfied when grouped execution is enabled.

Dynamic filter might filter data source entirely. In such case DELETE operation might fail if data source is not of UpdatablePageSource type.

Dynamic filter tuple domains might be relatively large and expensive to evaluate. They must be simplified so that dynamic filters do not incur additional CPU cost if they are not filtering much.

sopel39 added the WIP label Feb 11, 2020

cla-bot bot added the cla-signed label Feb 11, 2020

sopel39 force-pushed the ks/enable_dyn_filtering branch 10 times, most recently from 7c796df to 43e6420 Compare February 13, 2020 09:16

sopel39 changed the title ~~[WIP] enable dynamic filtering by default~~ enable dynamic filtering by default Feb 17, 2020

sopel39 removed the WIP label Feb 17, 2020

sopel39 requested review from findepi and martint February 17, 2020 09:40

raunaqmorarka reviewed Feb 19, 2020

View reviewed changes

Fix removal of dynamic filters within join filter condition

61d4609

sopel39 force-pushed the ks/enable_dyn_filtering branch from 43e6420 to 63c145b Compare February 22, 2020 20:46

sopel39 assigned findepi and martint and unassigned findepi Feb 22, 2020

martint approved these changes Feb 22, 2020

View reviewed changes

sopel39 added 6 commits March 2, 2020 12:47

Remove redundant isEnableDynamicFiltering check

147e9f7

When isEnableDynamicFiltering is disabled, then dynamic filters won't be added by planner.

Restore table scan printing of dynamic filters

8680834

Add check that dynamic filtering is not enabled with grouped execution

6782d7e

Dynamic filtering assumes that join build side is evaulated fixed (task_concurrency) amount of times. This constraint is not satisfied when grouped execution is enabled.

Don't match dynamic filters in FilterMatcher

26f118a

Remove obsolete TODO

cfae1a0

sopel39 added 4 commits March 2, 2020 12:54

Add support for dynamic filter to TableScanOperator

3240581

Use updatable page source when dynamic filter constraint is empty

341ec2f

Dynamic filter might filter data source entirely. In such case DELETE operation might fail if data source is not of UpdatablePageSource type.

Simplify dynamic filter before passing it to HivePageSourceFactory

8e86979

Dynamic filter tuple domains might be relatively large and expensive to evaluate. They must be simplified so that dynamic filters do not incur additional CPU cost if they are not filtering much.

Enable dynamic filtering by default

3541ae2

sopel39 force-pushed the ks/enable_dyn_filtering branch from 63c145b to 3541ae2 Compare March 2, 2020 11:54

sopel39 merged commit ddcbe76 into trinodb:master Mar 2, 2020

sopel39 deleted the ks/enable_dyn_filtering branch March 2, 2020 11:54

sopel39 mentioned this pull request Mar 2, 2020

Release notes for 331 #2880

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable dynamic filtering by default #2793

enable dynamic filtering by default #2793

sopel39 commented Feb 11, 2020

sopel39 commented Feb 17, 2020 •

edited

Loading

raunaqmorarka Feb 19, 2020

sopel39 Feb 22, 2020

martint left a comment

martint Feb 22, 2020

martint Feb 22, 2020

sopel39 Mar 2, 2020

martint Feb 22, 2020

martint Feb 22, 2020

martint Feb 22, 2020

		@@ -279,8 +279,6 @@ public PlanOptimizers(
		.add(new RemoveTrivialFilters())

		@@ -2042,7 +2042,7 @@ private PhysicalOperation createLookupJoin(
		ImmutableList.Builder<OperatorFactory> factoriesBuilder = new ImmutableList.Builder<>();

		@@ -117,7 +117,7 @@ public ConnectorPageSource createPageSource(ConnectorTransactionHandle transacti
		hiveSplit.getFileSize(),

enable dynamic filtering by default #2793

enable dynamic filtering by default #2793

Conversation

sopel39 commented Feb 11, 2020

sopel39 commented Feb 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

sopel39 commented Feb 17, 2020 •

edited

Loading