-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Migrate JDBC plugins to page source API #25123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5cc9309 to
d35009f
Compare
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSource.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSource.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSource.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSource.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSource.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSource.java
Outdated
Show resolved
Hide resolved
d35009f to
bfbff88
Compare
|
postgresql unpartitioned sf10.pdf
TPCH is mostly the same, TPCDS has some gains |
bfbff88 to
37d9551
Compare
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSource.java
Outdated
Show resolved
Hide resolved
| sqlException.setNextException(cause.getNextException()); | ||
| } | ||
| throw sqlException; | ||
| resultSet = requireNonNull(getFutureValue(resultSetFuture), "resultSet is null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there is SQLException failure, there may be a little difference.
if (e.getCause() instanceof SQLException cause) {
SQLException sqlException = new SQLException(cause.getMessage(), cause.getSQLState(), cause.getErrorCode(), e);
if (cause.getNextException() != null) {
sqlException.setNextException(cause.getNextException());
}
throw sqlException;
}
37d9551 to
b34f308
Compare
538888c to
d4acf20
Compare
56b1234 to
7ca021e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates JDBC connectors from a recordset-based implementation to the new page source API in order to enable dynamic row filtering and improved columnar filter evaluation optimizations. Key changes include:
- Migrating connector and split manager classes to use ConnectorPageSourceProvider instead of ConnectorRecordSetProvider.
- Updating tests to iterate over SourcePage rather than using a RecordCursor.
- Removing RemoteQueryCancellation modules and related test files.
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftUnloadConnector.java | Updated constructor parameter to use jdbcPageSourceProvider. |
| plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftSplitManager.java | Changed annotation from @ForRecordCursor to @ForJdbcClient for the executor. |
| plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftPageSourceProvider.java | Replaced RecordPageSource with jdbcPageSourceProvider.createPageSource. |
| plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftClientModule.java | Removed RemoteQueryCancellationModule installation. |
| plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClientModule.java | Removed RemoteQueryCancellationModule installation. |
| plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcPageSourceProvider.java | Updated test methods to use page source and adjusted iteration accordingly. |
| plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcPageSource.java | Updated tests to use JdbcPageSource and Surface new API validated through SourcePage iteration. |
| Others (modules, configurations, removed files) | Cleaned up RemoteQueryCancellation and deprecated recordset implementations. |
Comments suppressed due to low confidence (2)
plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftPageSourceProvider.java:69
- The error message still refers to 'recordSetProvider' instead of 'jdbcPageSourceProvider'. Please update the message accordingly.
this.jdbcPageSourceProvider = requireNonNull(jdbcPageSourceProvider, "recordSetProvider is null");
plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcPageSourceProvider.java:110
- The failure message should be updated to reference 'pageSource' instead of 'recordSet' to reflect the new API.
assertThat(pageSource).withFailMessage("recordSet is null").isNotNull();
7ca021e to
def42d8
Compare
This enables dynamic row filtering and columnar filter evaluation optimizations on JDBC based sources. We're also able to use io.trino.plugin.jdbc.JdbcPageSource#isBlocked to yield when waiting for results from source DB. RemoteQueryCancellation is removed because all jdbc queries are now executed on a separate thread pool which allows remote query cancellation.
def42d8 to
1c11826
Compare
|
@raunaqmorarka Are benchmarks ok now? |
Yes, i've updated my initial results, there aren't significant regressions any more |
|
@sourcery-ai review |
|
Hi @sajjoseph! 👋 Only authors and team members can run @sourcery-ai commands on public repos. |


Description
Changes JDBC connectors to produce pages rather a cursor.
This allows JDBC connectors to benefit from dynamic row filtering and columnar filter evaluation optimizations.
We're also able to use
io.trino.plugin.jdbc.JdbcPageSource#isBlockedto yield when waiting for results from source DB.RemoteQueryCancellation is removed because all jdbc queries are now executed
on a separate thread pool which allows remote query cancellation.
Additional context and related issues
I made a JMH benchmark at raunaqmorarka@6e015db
This assesses the performance of a highly selective filter (~1% selectivity) on a varchar column that is in front of another 7 varchar columns.
The cursor interface version is worse both with and without columnar filter evaluation enabled.
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: