Disable JDBC join pushdown

[Dith3r]

Description

Disable JDBC join pushdown as it could degrade performance.

Additional context and related issues

Release notes

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

# Section
* Fix some things. ({issue}`issuenumber`)

[findepi]

JdbcJoinPushdownSupportModule is used only by connectors which support cost-based join pushdown.

[Dith3r]

JdbcMetadataConfig your comment:

Join pushdown is disabled by default as this is the safer option.

And it is enabled by default.

[raunaqmorarka]

I'm assuming that meant that it was unsafe to enable in all cases but is safe if we do it based on the cost based estimate.
I think the cost based estimate doesn't account for reduction in probe output due to dynamic filtering. It might be possible to estimate this based on NDV of the columns being joined. @Dith3r maybe we can try building that into the cost estimation.
Otherwise, I don't think we should outright disable this as this covers a wide variety of data sources and we can't prove that disabling pushdown is the best strategy overall.

[sopel39]

I think the cost based estimate doesn't account for reduction in probe output due to dynamic filtering.

I think we need to consider different join types under assumption that we want to minimize data size fetched from connector

1). Filtering join (e.g. fact table join with dimension table), non-expanding

Filtering join are most common and should always be beneficial to pushdown. Even with perfect DF from Trino, join output will always be smaller than probe and build side

2). Expanding join (multiplies output rows for every probe input row), no filtering

Here DFs are irrelevant (no filtering). Might still be beneficial to pushdown depending on build side size. However, expanding joins are usually hard to estimate, so I would avoid pushing them

3). Expanding join that does some filtering also

Maybe we could estimate % of rows filtered and expanding factor for remaining rows? Generally, I would probably avoid pushdown of expanding joins because of uncertainity of stats. Assuming DFs work, then they should do good job at filtering probe side data so we should only get rows that get expanded. Pushing expanding join to connector might explode data produced by connector.

[findepi]

I'm assuming that meant that it was unsafe to enable in all cases but is safe if we do it based on the cost based estimate.

yes

[findepi]

What I'm essentially saying is that we probably could simplify the cost decision to expanding vs non-expanding join.

I believe that's the current logic as implemented.

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

Lines 26 to 28 in 095d35a

	// Normally we would put 1.0 as a default value here to only allow joins which do not expand data to be pushed down.
	// We use 1.25 to adjust for the fact that NDV estimations sometimes are off and joins which should be pushed down are not.
	private double joinPushdownAutomaticMaxJoinToTablesRatio = 1.25;

However, by not-pushing join we might prevent further pushdowns

true

Hence it's not entirely connector decision do do or not do pushdown

that would require exploratory optimizer

[sopel39]

that would require exploratory optimizer

yes, but not full exploratory optimizer, but it would be sufficient to have bunch of alternatives in source stage cc. @assaf2

[Dith3r]

For SQL databases, we should only consider push down if a covering index is present and for Postgres if any of the joined tables is not partitioned, as this case underperforms.

[sopel39]

For SQL databases, we should only consider push down if a covering index is present and for Postgres if any of the joined tables is not partitioned, as this case underperforms.

I think it's something we should put into documentation (cc @mosabua ) that users should add indexes on columns they plan to use in joins

[findepi]

If this is indeed the case, then first and foremost the join pushdown code should check for necessary indexes being present.

[sopel39]

I think we should investigate more why performance is degraded

[mosabua]

I think removal because it could degrade performance is not correct. It could (and does) also improve performance in many cases and as such it should stay on in the base connector. Individual connectors can then potentially override it to either always be disabled, change the default to be disabled or enabled, and also provide a user configurable option.

Once we know when and why performance it degraded we can make a more informed decision how to proceed. Do you have any specific about the problems you are experiencing and caused you to send this PR @Dith3r ?

[Dith3r]

PostgreSQL sf10 partitioned with analyzed tables suffers 3x degradation of performance (in some rare queries, degradation is 8-10x). With other PostgreSQL benchmarks, I've not seen any degradation/improvement from this option set to enable. I will try to investigate it further.

[hashhar]

@Dith3r Have you checked if the Postgres connector provides stats for partitioned tables at all? IIRC we haven't implemented it with partitioned tables in mind so either the stats are entirely missing or incorrect.

Do you see same issue with unpartitioned tables?

[Dith3r]

Unpartitioned tables does not suffer from join push down in ppstgresql. For partitioned tables connector provides estimates and it triggers push down which is not handled well.