-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Jdbc merge support #20532
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
Jdbc merge support #20532
Conversation
|
@wendigo @vlad-lyutenko @hashhar would you like to review this PR? |
|
@electrum Would you mind to have a look when you have sometime? thanks |
hashhar
left a comment
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.
I looked at the implementation pieces, only skimmed the test code.
Have left some initial comments, the overall mechanism appears "correct" to me % details like column quoting, having to fetch metadata multiple times within a single query (higher chances of inconsistency because of concurrent changes in underlying DB) etc.
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcTableHandle.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMergeSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSourceProvider.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java
Outdated
Show resolved
Hide resolved
|
Postgresql tests failures due to OOM will be resolved #21127 |
|
Can you please expand description section of the PR with high level overview of approach taken? |
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Hey @chenjian2664, @mosabua, @findepi any update on this PR? We would love to use this feature w/ dbt-trino adapter. |
Waiting for the review |
|
cc @djsstarburst @electrum for more MERGE support |
|
Hey team, any update on the status of this PR? |
|
Would love to have merge support for postgresql connector, as Update with help of from query is not directly supported. |
|
We are trying to give this priority in the review and merge queue at the moment. We definitely plan on getting this in. Timing is to be determined though. |
Also removes two-pass encoding in favor of single pass conversion logic to avoid extra allocations
kokosing
left a comment
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.
Some incrementa comments
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
Outdated
Show resolved
Hide resolved
Introduce JdbcPageSourceProvider and refactor phoenix5 MERGE implementation based on it
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcTableHandle.java
Show resolved
Hide resolved
plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteClient.java
Show resolved
Hide resolved
plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteMergeTableHandle.java
Show resolved
Hide resolved
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jason.
|
|
@chenjian2664 Any update on that issue? |
|
@shohamyamin for some reason, we move the working on #23034 now. |
Description
This is a follow up of #16693
It mainly implements several important interfaces in the https://trino.io/docs/current/develop/supporting-merge.html#connector-support-for-merge document:
ConnectorMergeSinkAPI. InstoreMergedRows, the page data is classified intoINSERT,UPDATE,DELETEthree categories, and then call the correspondingSinkclass to operate the corresponding Connector.INSERTdata is processed through the existingJdbcPageSink.UPDATEandDELETEtype data are modified through the primary keys. The implementation reuses theJdbcPageSinkcode and only changes the executed sql statement. The syntax used when deleting data using primary key constraints in each database may be different, so when creating the Sink class,mergeRowIdConjunctswill be obtained from BaseJdbcClient (BaseJdbcClient#buildMergeRowIdConjuncts)getMergeRowIdColumnHandleAPI. For connectors that support Merge, return a RowType containing all primary key column information, otherwise return as original.beginMergeAPI. ReturnsJdbcMergeTableHandle, which contains all columns that need to be scanned, (implemented inBaseJdbcClient#updatedScanColumnsForMerge), and the tableHandle of Insert.In the third commit, FTE needs to be supported, so the
finishMergeAPI is also implemented. Write all insert+delete data to the temporary table first, and then update the target table.Additional context and related issues
There are also some details in #16944
Release notes
(x) Release notes are required, with the following suggested text: