Determine automatically if push join to table scan by losipiuk · Pull Request #6818 · trinodb/trino

losipiuk · 2021-02-04T13:04:10Z

~~On top of: #6752~~

~~Review last commit only.~~

raunaqmorarka · 2021-02-04T13:32:30Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushJoinIntoTableScan.java

What if one of left output count or right count is known and larger than join output row count, why not pushdown join in such case as well ?

Yeah - we could. Though it is strictly theoretical case. As if we do not know either left or right size. We would not know the join size :)

Ah right, I missed that. Any particular reason for basing this on row count instead of size ?

Not really. Probably size would be more appropriate. I will see how painful it is to change that.

findepi · 2021-02-05T09:36:59Z

On top of: #6752

i plan to review this once that one is merged

findepi · 2021-02-10T08:59:00Z

core/trino-main/src/main/java/io/trino/sql/analyzer/FeaturesConfig.java

it is safe to make it the default

findepi · 2021-02-10T09:00:27Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushJoinIntoTableScan.java

Since stats calculation can be costly (eg can involve a trip to metastore), short-circuit calculation as early as you can.
To keep this readable, please extract the condition to a separate method.

findepi · 2021-02-10T09:01:13Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushJoinIntoTableScan.java

ideally use a switch, and make it exhaustive, future proofing for the case when we add something like AUTOMATIC_EAGER (which we don't have to add yet, but we may want to add in the future)

nvrm, in this case it doesn't matter -- this is the only place the enum is used, so no way it gets forgotten and not updated

findepi · 2021-02-10T09:02:52Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushJoinIntoTableScan.java

While this is not a rocket science, it'd be nice to add some comment, eg why we're choosing + over max.
from my perspective it was some 'random thought from findepi' (and i don't feel strongly), but still let's safe future readers suffering and try to word some explanation.

I added some reasoning. Not sure if helpful

losipiuk · 2021-02-10T16:10:20Z

ac

findepi · 2021-02-11T08:39:17Z

core/trino-main/src/main/java/io/trino/sql/analyzer/FeaturesConfig.java

see conversation about code level documentation in the other pr

Added comment as a separate commit before introducing AUTOMATIC mode.

findepi · 2021-02-11T08:40:00Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushJoinIntoTableScan.java

I think the getJoinPushdownMode should be consulted inside shouldProceedWithPushDown
(or you'd want to rename the method to indicate it's appropriate for "automatic" mode only)

Renamed the method to skipJoinPushdownBasedOnCost (reversing true/false return value semantics), and moved getJoinPushdownMode(context.getSession()) == JoinPushdownMode.AUTOMATIC inside.

Add "automatic" mode of join pushdown operation. In that mode join will only be pused down into table scan if statistics are available for join node and both source table scan nodes. And if expected numuber of rows coming out of join is less than total number of rows from both sources.

sopel39 · 2021-02-11T12:17:34Z

core/trino-main/src/main/java/io/trino/sql/analyzer/FeaturesConfig.java

@@ -135,16 +135,7 @@ public class FeaturesConfig
    private DataSize filterAndProjectMinOutputPageSize = DataSize.of(500, KILOBYTE);


Even if number of rows after pushdown is smaller then without pushdown it could significantly increase cpu overhead of underlying source (table scans might be much cheaper than join). I think it would be great to determine what's the impact of pushdown on underlying connectors. It could be that join pushdown is beneficial only when joins are very non selective and users don't want cpu of underlying connector to increase significantly.

Agreed. Yet I would assume that you will still be able to disable pushdown on per-connector level in configuration. As well as per-query using session.

Totally -- #6874 provides both catalog level config and session toggle.

sopel39 · 2021-02-11T12:23:59Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushJoinIntoTableScan.java

+            return true;
+        }
+
+        if (joinOutputSize > leftOutputSize + rightOutputSize) {


Consider adding some factor here, e.g pushed down join should produce 2x less rows than in trino. Such factor might need to be empirically established

so you mean to replace left + right with max(left, right) * 0.5? Works for me, given that the current formula is not very scientificly determined.
I think we should do "something reasonable" & iterate.

Yeah - I find initial value of a factor 1.0 as good as 0.5

cla-bot bot added the cla-signed label Feb 4, 2021

losipiuk requested a review from findepi February 4, 2021 13:04

losipiuk force-pushed the lo/oportunistic-join-pushdown-cost-based branch from 5513f17 to 92dbbb0 Compare February 4, 2021 13:18

raunaqmorarka reviewed Feb 4, 2021

View reviewed changes

losipiuk force-pushed the lo/oportunistic-join-pushdown-cost-based branch from 92dbbb0 to 0e8b453 Compare February 9, 2021 15:03

findepi approved these changes Feb 10, 2021

View reviewed changes

losipiuk force-pushed the lo/oportunistic-join-pushdown-cost-based branch from 0e8b453 to 70680d7 Compare February 10, 2021 16:10

losipiuk mentioned this pull request Feb 10, 2021

Add optimizer rule to pushdown join to connector #6752

Merged

findepi approved these changes Feb 11, 2021

View reviewed changes

losipiuk added 2 commits February 11, 2021 12:40

Add comment on default value for optimizer.join-pushdown

94b2285

losipiuk force-pushed the lo/oportunistic-join-pushdown-cost-based branch from 70680d7 to 2e3ebdf Compare February 11, 2021 11:43

sopel39 reviewed Feb 11, 2021

View reviewed changes

losipiuk closed this Feb 24, 2021

		@@ -135,16 +135,7 @@ public class FeaturesConfig
		private DataSize filterAndProjectMinOutputPageSize = DataSize.of(500, KILOBYTE);

Conversation

losipiuk commented Feb 4, 2021 • edited by findepi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raunaqmorarka Feb 4, 2021 • 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!

raunaqmorarka Feb 4, 2021 • 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!

findepi commented Feb 5, 2021

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!

losipiuk commented Feb 10, 2021

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 Feb 11, 2021 • 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!

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!

4 participants

losipiuk commented Feb 4, 2021 •

edited by findepi

Loading

raunaqmorarka Feb 4, 2021 •

edited

Loading

raunaqmorarka Feb 4, 2021 •

edited

Loading

sopel39 Feb 11, 2021 •

edited

Loading