Add preferred timeout for small dynamic filters by Dith3r · Pull Request #22527 · trinodb/trino

Dith3r · 2024-06-27T12:15:31Z

Description

Add a CBO rule to estimate if dynamic filter is worth waiting for. Focus on small tables that are most often used as dimension tables and nodes that generate small number of distinct values.

Rule DeterminePreferredDynamicFilterTimeout could allow us to remove 20s forced wait time for every possible DF in JDBC connectors if table presents the least row count statistic.

Query	Baseline duration[s]	PR duration[s]
tpcds/q59	12.10	4.55
tpcds/q02	9.63	3.47
tpcds/q36	5.82	2.60
tpcds/q53	8.04	4.80
tpcds/q13	7.7	5.1

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text:

# General
* Improve performance of queries with selective joins. ({issue}`22527`)

core/trino-main/src/main/java/io/trino/SystemSessionProperties.java

core/trino-main/src/main/java/io/trino/execution/DynamicFilterConfig.java

...src/main/java/io/trino/sql/planner/optimizations/DeterminePreferredDynamicFilterTimeout.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcDynamicFilteringConfig.java

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestBackgroundHiveSplitLoader.java

core/trino-main/src/main/java/io/trino/sql/DynamicFilters.java

core/trino-main/src/main/java/io/trino/sql/planner/planprinter/PlanPrinter.java

core/trino-main/src/main/java/io/trino/SystemSessionProperties.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcDynamicFilteringSplitManager.java

martint · 2024-07-16T14:56:47Z

...src/main/java/io/trino/sql/planner/optimizations/DeterminePreferredDynamicFilterTimeout.java

Why isn’t this a Rule or set of them? Visitor-based optimizers are legacy, and we’re moving away from them. We should avoid introducing any new ones.

We're trying to change an attribute of the filter node on the probe side scan of join based on the estimated size of the build side of that join. I'm not sure how this would be designed as one or more series of Rules.
Do we have a similar Rule elsewhere that we could use as a reference ?

Rule approach would require matching against Join/SemiJoin/DynamicFilterSourceNode and processing all probe side FilterNodes, extracting conjuncts to match against join node dynamic filter. It would cause far more lookups and rewrites than extracting all dynamic filter sources and process filter nodes once.

github-actions · 2024-08-07T17:03:13Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

wendigo · 2024-08-07T18:34:08Z

@Dith3r can you rebase?

wendigo · 2024-08-08T05:44:50Z

@sopel39 ptal

sopel39 · 2024-08-08T09:32:31Z

I think @martint has some architecture comments for this PR. @martint are you OK with approach here?

github-actions · 2024-08-29T17:18:06Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

wendigo · 2024-08-29T17:52:02Z

@Dith3r please rebase :)

sopel39 · 2024-09-23T10:22:00Z

core/trino-spi/src/main/java/io/trino/spi/connector/DynamicFilter.java

@Dith3r could you remind me why it was the case?

If you call getPreferredTimeout before acquiring future from isBlocked you can receive future with excluded already resolved DF associated with preferred timeout and wait for DFs that have no preferred timeout set.

If DF is already resolved, then no waiting will happen anyway, right?

Add a CBO rule to estimate if dynamic filter is worth waiting for. Focus on small tables that are most often used as dimension tables.

sopel39 · 2024-10-14T13:15:59Z

core/trino-main/src/main/java/io/trino/server/DynamicFilterService.java

+                        DynamicFilters.Descriptor::getId,
+                        DynamicFilters.Descriptor::getPreferredTimeout,
+                        (timeout1, timeout2) -> {
+                            if (timeout1.isPresent() && timeout2.isPresent()) {


you could use io.trino.util.Optionals#combine

It would require similar function which is based on OptionalLong.

sopel39 · 2024-10-14T13:34:45Z

...src/main/java/io/trino/sql/planner/optimizations/DeterminePreferredDynamicFilterTimeout.java

+                return !isAtMostScalar(joinNode.getRight()) && !isAtMostScalar(joinNode.getLeft());
+            }
+
+            // Skip for joins with multi keys since output row count stats estimation can wrong due to


can wrong -> can be wrong

sopel39 · 2024-10-14T13:37:35Z

...src/main/java/io/trino/sql/planner/optimizations/DeterminePreferredDynamicFilterTimeout.java

+            }
+
+            // Skip for joins with multi keys since output row count stats estimation can wrong due to
+            // low correlation between multiple join keys.


it should probably: can be wrong due to potentially high correlation between join columns, which might lead to underestimation of join output row count

sopel39 · 2024-10-14T13:52:27Z

...src/main/java/io/trino/sql/planner/optimizations/DeterminePreferredDynamicFilterTimeout.java

+    private static boolean isExpandingPlanNode(PlanNode node)
+    {
+        return node instanceof JoinNode
+               // consider union node and exchange node with multiple sources as expanding since it merge the rows


should be more like union and exchange nodes that consume from multiple source are considered expanding since they produce more data than their children individually

sopel39 · 2024-10-14T13:54:05Z

...src/main/java/io/trino/sql/planner/optimizations/DeterminePreferredDynamicFilterTimeout.java


        private DynamicFilterTimeout getBuildSideState(PlanNode planNode, Symbol dynamicFilterSymbol)
        {
+            if (isAtMostScalar(planNode)) {


comment would be nice + commit message

sopel39 · 2024-10-14T16:55:21Z

...src/main/java/io/trino/sql/planner/optimizations/DeterminePreferredDynamicFilterTimeout.java

+                .matches();
+    }
+
+    private static boolean isExpandingPlanNode(PlanNode node)


should be moved below usage

cla-bot bot added the cla-signed label Jun 27, 2024

Dith3r requested review from martint, raunaqmorarka and sopel39 June 27, 2024 12:15

github-actions bot added iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector mongodb MongoDB connector labels Jun 27, 2024

Dith3r force-pushed the ke/df-timeout branch 6 times, most recently from 3e150d1 to 6ee8f40 Compare July 3, 2024 19:23

Dith3r force-pushed the ke/df-timeout branch 4 times, most recently from a70fc00 to c970010 Compare July 16, 2024 06:08

raunaqmorarka reviewed Jul 16, 2024

View reviewed changes

Dith3r force-pushed the ke/df-timeout branch from c970010 to 9ec82d0 Compare July 16, 2024 09:46

Dith3r requested a review from raunaqmorarka July 16, 2024 09:52

Dith3r force-pushed the ke/df-timeout branch from 9ec82d0 to 293ed94 Compare July 16, 2024 10:12

raunaqmorarka approved these changes Jul 16, 2024

View reviewed changes

core/trino-main/src/main/java/io/trino/SystemSessionProperties.java Outdated Show resolved Hide resolved

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcDynamicFilteringSplitManager.java Outdated Show resolved Hide resolved

raunaqmorarka added the performance label Jul 16, 2024

Dith3r force-pushed the ke/df-timeout branch 3 times, most recently from 41e1320 to 5238c35 Compare July 16, 2024 11:09

martint reviewed Jul 16, 2024

View reviewed changes

Dith3r force-pushed the ke/df-timeout branch 2 times, most recently from 20ba16f to 09b8a31 Compare July 17, 2024 07:16

github-actions bot added the stale label Aug 7, 2024

Dith3r force-pushed the ke/df-timeout branch from 09b8a31 to faf674e Compare August 7, 2024 19:04

github-actions bot removed the stale label Aug 8, 2024

github-actions bot added the stale label Aug 29, 2024

github-actions bot removed the stale label Aug 30, 2024

sopel39 mentioned this pull request Sep 18, 2024

Materialize tables during planning #23426

Closed

sopel39 reviewed Sep 23, 2024

View reviewed changes

Dith3r closed this Oct 14, 2024

Dith3r force-pushed the ke/df-timeout branch from faf674e to e0e83a4 Compare October 14, 2024 10:49

Dith3r reopened this Oct 14, 2024

Dith3r force-pushed the ke/df-timeout branch from 6ab1f97 to 3939835 Compare October 14, 2024 11:41

Dith3r added 3 commits October 14, 2024 14:57

Add preferred timeout for small dynamic filters

497c6f4

Add a CBO rule to estimate if dynamic filter is worth waiting for. Focus on small tables that are most often used as dimension tables.

Use static import in TestDeterminePreferredDynamicFilterTimeout

98669ac

Add DF preferred timeout for scalar build side

40ab76e

Dith3r force-pushed the ke/df-timeout branch from 3939835 to 40ab76e Compare October 14, 2024 12:58

sopel39 reviewed Oct 14, 2024

View reviewed changes

Dith3r closed this Nov 7, 2024

Conversation

Dith3r commented Jun 27, 2024 • edited by raunaqmorarka Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 7, 2024

Uh oh!

wendigo commented Aug 7, 2024

Uh oh!

wendigo commented Aug 8, 2024

Uh oh!

sopel39 commented Aug 8, 2024

Uh oh!

github-actions bot commented Aug 29, 2024

Uh oh!

wendigo commented Aug 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sopel39 Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

Dith3r commented Jun 27, 2024 •

edited by raunaqmorarka

Loading

sopel39 Oct 14, 2024 •

edited

Loading