Enable dynamic filtering in JDBC connector by jerryleooo · Pull Request #8137 · trinodb/trino

jerryleooo · 2021-05-29T08:46:58Z

#7968

depends on #8046

ebyhr

Left some initial comments.

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

...rino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DynamicFilteringJdbcSessionProperties.java

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

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

...rino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DynamicFilteringJdbcSessionProperties.java

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

jerryleooo · 2021-06-14T05:12:38Z

@ebyhr updated as commented, can see if need further review, thanks for the hard work!

...rino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DynamicFilteringJdbcSessionProperties.java

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

...rino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DynamicFilteringJdbcSessionProperties.java

jerryleooo · 2021-06-17T14:08:20Z

Hi @ebyhr @sopel39 thanks for the hard work and can help suggest what I still need to do to get this PR approved?

haldes · 2021-06-26T03:36:35Z

@jerryleooo Thanks for this feature. We also want to use this feature for few of the massive join use case. Waiting for this ticket to be merged.
Also wanted to understand in case the build side emits a large no. of keys (say a 100K keys), how will this behave. For columns like varchar, integers , how will these keys be pushed to the probe side? Databases like PostgresSQL has a IN clause limit of 32767. So will it split the 100K keys into batches of 32767?

jerryleooo · 2021-06-26T08:48:10Z

Hi @haldes thanks for the interest!
I think "dynamic filtering" has some configurations about the maximum values on per driver, like max-distinct-values-per-driver and max-size-per-driver. Can check them on https://trino.io/docs/current/admin/dynamic-filtering.html

Correct me if I misunderstood @sopel39 @findepi @ebyhr

wolfgangcolsman · 2021-07-16T11:29:53Z

For the "massive join case" I created #8519. I kept this separate as this is a principle issue, not only related to joins.
One issue we found when developing this functionality is the tuple domain simplification on VACHAR values when they contain random text such as guids that are not suitable for range queries. We configured compaction threashold to use Integer.MAX

jibinpt · 2021-07-26T21:18:43Z

@jerryleooo Thanks for working on this feature. Would like to use this feature. Is there any chance that this feature would be merged to Trino master soon? Was able to see dynamic filtering kicking in after CP the commits.

wolfgangcolsman · 2021-12-02T14:46:30Z

What needs to be done to merge this? Can we move this forward?

lhofhansl · 2022-06-08T00:37:31Z

Just started looking into this (see my slack comment on dev). Then found your PR (I started almost exactly the same way.) Let's finish one and get it in.
I'll try this out today.

lhofhansl · 2022-06-08T05:46:50Z

I had a fix up a few things, but this works nicely. Should we finish this?

jerryleooo · 2022-06-08T08:16:48Z

Thanks, @lhofhansl, I just rebased the code to resolve the conflict, let's see how the organization thinks?

lhofhansl · 2022-06-08T16:13:28Z

...rc/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorRecordSetProvider.java

+    @Override
+    public RecordSet getRecordSet(ConnectorTransactionHandle transaction, ConnectorSession session, ConnectorSplit split, ConnectorTableHandle table, List<? extends ColumnHandle> columns, DynamicFilter dynamicFilter)
+    {
+        return getRecordSet(transaction, session, split, table, columns);


This should actually pass the DynamicFIlter along.

Yes, should be something like

try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { return new ClassLoaderSafeRecordSet(delegate.getRecordSet(transaction, session, split, table, columns, dynamicFilter), classLoader); }

lhofhansl · 2022-06-08T16:15:13Z

plugin/trino-phoenix/src/main/java/io/trino/plugin/phoenix/PhoenixClientModule.java

        configBinder(binder).bindConfig(JdbcMetadataConfig.class);
        configBinder(binder).bindConfig(JdbcWriteConfig.class);

+        configBinder(binder).bindConfig(DynamicFilteringJdbcConfig.class);


Is this needed?

I'm guessing this is needed because phoenix doesn't derive from JdbcPlugin.
Changes for Phoenix connector should go into separate commit and need to be explicitly covered by query runner tests.

lhofhansl · 2022-06-08T16:16:46Z

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

+
+public class DynamicFilteringJdbcConfig
+{
+    private boolean enableDynamicFiltering;


Default off? Should it be on?

We can turn it on in a follow-up change after running benchmarks on it. We'll also want tests with actual connectors rather than just H2QueryRunner before we turn it on by default.

lhofhansl · 2022-06-08T16:28:43Z

@raunaqmorarka FYI :)

lhofhansl · 2022-06-08T16:59:24Z

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

+    public RecordSet getRecordSet(ConnectorTransactionHandle transaction, ConnectorSession session, ConnectorSplit split, ConnectorTableHandle table, List<? extends ColumnHandle> columns, DynamicFilter dynamicFilter)
+    {
+        if (isEnableDynamicFiltering(session)) {
+            long timeoutMillis = getDynamicFilteringWaitTimeout(session).toMillis();


I find some weird behavior when trying this Postgres as a probe side. It almost seems random whether dynamic filtering is used or not.

I've set join_reordering_strategy='NONE' and also set dynamic_filtering_wait_timeout='10000ms'.

Executing the same join over and over I find some sometimes it uses dynamic filters and sometimes it does not.

In the debugger I see that a dynamic filter is passed here, but its tupledomain is "all".
I think this is outside of this code, but wondering if you have seen this too.
(The performance difference between the two cases is 250ms vs 38 seconds.)

All single node between two different JDBC datasources.

Update: This happens only with PARTITIONED joins... As expected I guess since I am testing single node.

Is this waiting for something? Looks good to me.

lhofhansl · 2022-06-21T21:00:12Z

Ping... Seems to be quite useful, especially when Trino is used as a general query platform between many big and small data sources.

raunaqmorarka · 2022-06-28T04:48:00Z

...rc/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorRecordSetProvider.java

+    @Override
+    public RecordSet getRecordSet(ConnectorTransactionHandle transaction, ConnectorSession session, ConnectorSplit split, ConnectorTableHandle table, List<? extends ColumnHandle> columns, DynamicFilter dynamicFilter)
+    {
+        return getRecordSet(transaction, session, split, table, columns);


Yes, should be something like

try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { return new ClassLoaderSafeRecordSet(delegate.getRecordSet(transaction, session, split, table, columns, dynamicFilter), classLoader); }

raunaqmorarka · 2022-06-28T04:48:54Z

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

+
+public class DynamicFilteringJdbcConfig
+{
+    private boolean enableDynamicFiltering;


We can turn it on in a follow-up change after running benchmarks on it. We'll also want tests with actual connectors rather than just H2QueryRunner before we turn it on by default.

raunaqmorarka · 2022-06-28T04:52:01Z

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

+public class DynamicFilteringJdbcConfig
+{
+    private boolean enableDynamicFiltering;
+    private Duration dynamicFilteringWaitTimeout = new Duration(0, TimeUnit.MINUTES);


Let's use a default of 20 seconds here, that will ensure that DF collection has sufficient time to complete without hanging too long. Also, if the build side gets too big, the DF collection will automatically bail out sooner.

raunaqmorarka · 2022-06-28T04:55:38Z

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

        this.nextSyntheticColumnId = nextSyntheticColumnId;
    }

+    public JdbcTableHandle withConstraint(TupleDomain<ColumnHandle> newConstraint)


withConstraint -> intersectedWithConstraint

raunaqmorarka · 2022-06-28T05:00:32Z

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

+        if (newDomain == constraint) {
+            return this;
+        }


This condition isn't really needed, we can keep it simple and remove it

raunaqmorarka · 2022-06-28T05:35:53Z

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

+                return getRecordSet(transaction, session, split, handle, columns);
+            }
+
+            try {


Please remove the try/catch we shouldn't be hiding any errors that result from DF pushdown

raunaqmorarka · 2022-06-28T05:46:46Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcDynamicFiltering.java

+    }
+
+    @Test
+    public void testSemiJoinDynamicFilteringNone()


By default semi joins with the query shapes we're using here get rewritten to inner joins by the optimizer. We don't need explicit testing of semi join in these tests, so you can just remove semijoin tests.

raunaqmorarka · 2022-06-28T05:54:33Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcDynamicFiltering.java

+                1, 1, 1);
+    }
+
+    private void assertDynamicFiltering(@Language("SQL") String selectQuery, Session session, int expectedRowCount, int... expectedOperatorRowsRead)


We don't need to assert on specific values of expectedRowCount and expectedOperatorRowsRead here.
We should run the query with DF explicitly disabled, getPhysicalInputPositions from QueryStats and assert that the value is less than the same value from running the query with DF enabled.
We should also use assertEqualsIgnoreOrder to ensure that results are the same with DF enabled/disabled.

raunaqmorarka · 2022-06-28T06:06:51Z

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

+            }
+
+            try {
+                dynamicFilter.isBlocked().get(timeoutMillis, MILLISECONDS);


This will block an execution thread in the engine and prevent other splits from running, so we need to avoid this somehow.
You can apply dynamic filtering in JdbcSplitManager instead and use ConnectorSplitSource#getNextBatch to provide splits with DF using a future there. Changes to ConnectorRecordSetProvider SPI won't be necessary then.

raunaqmorarka · 2022-06-28T06:11:34Z

plugin/trino-phoenix/src/main/java/io/trino/plugin/phoenix/PhoenixClientModule.java

        configBinder(binder).bindConfig(JdbcMetadataConfig.class);
        configBinder(binder).bindConfig(JdbcWriteConfig.class);

+        configBinder(binder).bindConfig(DynamicFilteringJdbcConfig.class);


I'm guessing this is needed because phoenix doesn't derive from JdbcPlugin.
Changes for Phoenix connector should go into separate commit and need to be explicitly covered by query runner tests.

raunaqmorarka · 2022-07-25T10:13:13Z

Thank you for your work on this. I'm closing this in favour of #13334 which will implement this functionality with a more holistic approach and more complete testing.

cla-bot bot added the cla-signed label May 29, 2021

jerryleooo marked this pull request as draft May 29, 2021 08:47

jerryleooo changed the title ~~implement dynamic filtering for ConnectorRecordSetProvider~~ Enable dynamic filtering for JDBC connectors May 29, 2021

jerryleooo changed the title ~~Enable dynamic filtering for JDBC connectors~~ Enable dynamic filtering in JDBC connector May 29, 2021

jerryleooo force-pushed the impl-jdbc-dynamic-filtering branch 2 times, most recently from 74c2852 to fb02adb Compare June 3, 2021 15:06

jerryleooo marked this pull request as ready for review June 3, 2021 15:06

jerryleooo requested a review from findepi June 3, 2021 15:06

jerryleooo marked this pull request as draft June 4, 2021 04:12

jerryleooo force-pushed the impl-jdbc-dynamic-filtering branch from 2fa8276 to f98a4d1 Compare June 7, 2021 05:44

jerryleooo marked this pull request as ready for review June 7, 2021 05:44

jerryleooo requested review from electrum, lhofhansl and raunaqmorarka June 7, 2021 05:57

jerryleooo added the jdbc Relates to Trino JDBC driver label Jun 9, 2021

jerryleooo requested a review from ebyhr June 9, 2021 08:48

ebyhr added the needs-docs This pull request requires changes to the documentation label Jun 12, 2021

ebyhr reviewed Jun 12, 2021

View reviewed changes

sopel39 reviewed Jun 14, 2021

View reviewed changes

...rino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DynamicFilteringJdbcSessionProperties.java Show resolved Hide resolved

sopel39 reviewed Jun 15, 2021

View reviewed changes

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

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

sopel39 reviewed Jun 15, 2021

View reviewed changes

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

jerryleooo requested review from ebyhr and sopel39 June 16, 2021 08:55

jerryleooo mentioned this pull request Jun 25, 2021

Roadmap of being a "gateway" for other data sources #8386

Open

3 tasks

jerryleooo mentioned this pull request Jul 16, 2021

Add support for dynamic filters in jdbc client #8523

Closed

jerryleooo mentioned this pull request Jul 30, 2021

Enable dynamic filtering in JDBC connector #7968

Closed

2 tasks

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:13

ebyhr removed the jdbc Relates to Trino JDBC driver label Aug 16, 2021

jerryleooo added 2 commits June 8, 2022 15:17

enable dynamic filtering in JDBC connector

811ac4a

update code and fix styles

cd1674c

jerryleooo force-pushed the impl-jdbc-dynamic-filtering branch from 1be1a71 to cd1674c Compare June 8, 2022 08:14

lhofhansl reviewed Jun 8, 2022

View reviewed changes

raunaqmorarka added the performance label Jun 28, 2022

raunaqmorarka reviewed Jun 28, 2022

View reviewed changes

raunaqmorarka mentioned this pull request Jun 28, 2022

Add DynamicFilter to ConnectorRecordSetProvider SPI #8046

Closed

raunaqmorarka closed this Jul 25, 2022

Conversation

jerryleooo commented May 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebyhr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerryleooo commented Jun 14, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerryleooo commented Jun 17, 2021

Uh oh!

haldes commented Jun 26, 2021

Uh oh!

jerryleooo commented Jun 26, 2021

Uh oh!

wolfgangcolsman commented Jul 16, 2021

Uh oh!

jibinpt commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangcolsman commented Dec 2, 2021

Uh oh!

lhofhansl commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lhofhansl commented Jun 8, 2022

Uh oh!

jerryleooo commented Jun 8, 2022

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!

lhofhansl commented Jun 8, 2022

Uh oh!

lhofhansl Jun 8, 2022 • 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!

lhofhansl Jun 14, 2022 • 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!

lhofhansl commented Jun 21, 2022

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!

jerryleooo commented May 29, 2021 •

edited

Loading

jibinpt commented Jul 26, 2021 •

edited

Loading

lhofhansl commented Jun 8, 2022 •

edited

Loading

lhofhansl Jun 8, 2022 •

edited

Loading

lhofhansl Jun 14, 2022 •

edited

Loading

raunaqmorarka commented Jul 25, 2022 •

edited

Loading