Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Jdbc merge support #23034

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Jdbc merge support #23034

merged 2 commits into from
Dec 9, 2024
+1,295 −638

Conversation

chenjian2664
Copy link
Contributor

@chenjian2664 chenjian2664 commented Aug 14, 2024 •
edited by kokosing
Loading

Description

Close #20532

Additional context and related issues

Release notes

(x) Release notes are required, with the following suggested text:

# PostgresSQL Jdbc
* Adding non-transactional support for {doc}`MERGE statements </sql/merge>`.

@cla-bot cla-bot bot added the cla-signed label Aug 14, 2024
@github-actions github-actions bot added the docs label Aug 14, 2024
@chenjian2664 chenjian2664 self-assigned this Aug 14, 2024
@chenjian2664 chenjian2664 force-pushed the jdbc_merge_support branch 2 times, most recently from 09df173 to dedee86 Compare August 14, 2024 09:57
@chenjian2664 chenjian2664 marked this pull request as ready for review August 14, 2024 14:20
@chenjian2664 chenjian2664 requested a review from kokosing August 14, 2024 15:06
@chenjian2664 chenjian2664 force-pushed the jdbc_merge_support branch 2 times, most recently from 8b2f8c8 to b6e0023 Compare August 16, 2024 11:06
kokosing
kokosing reviewed Aug 20, 2024
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another round of review. Please have a lot of patience, I still try to get deeper and still I am not there. There could be a case where some my comments could be reverted in future. It is a creative process, so I hope in the end we will get into a good spot with this PR, but it may require trials and errors.

I mostly reviewed the first commit. I asked for some preparatory commits, potentially we could do them as separate PR and merge it faster. Then we could rebase this PR.

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/JdbcClient.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/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/JdbcMergeSink.java Outdated Show resolved Hide resolved
plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteMergeTableHandle.java Outdated Show resolved Hide resolved
plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteMetadata.java Outdated Show resolved Hide resolved
plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteMetadata.java Outdated Show resolved Hide resolved
plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java Outdated Show resolved Hide resolved
@chenjian2664 chenjian2664 force-pushed the jdbc_merge_support branch 7 times, most recently from 159b755 to fb71994 Compare August 22, 2024 06:55
@kokosing kokosing mentioned this pull request Aug 25, 2024
Merged
@chenjian2664 chenjian2664 mentioned this pull request Sep 1, 2024
Merged
@chenjian2664 chenjian2664 marked this pull request as draft September 9, 2024 06:08
@chenjian2664 chenjian2664 force-pushed the jdbc_merge_support branch 3 times, most recently from db01ce8 to 4a3fcb5 Compare September 9, 2024 07:39
@chenjian2664 chenjian2664 marked this pull request as ready for review September 9, 2024 08:21
@chenjian2664
Copy link
Contributor Author

chenjian2664 commented Sep 9, 2024 •
edited
Loading

@kokosing Here's the updated version for review
Thanks for the guidance in the previous reviews
I removed the merge support for the Postgresql connector, I am planning separate it as the follow up.

@chenjian2664
Copy link
Contributor Author

chenjian2664 commented Sep 10, 2024 •
edited
Loading

Add support for postgresql.
Separate merge with fte for postgresql on #23345

@chenjian2664 chenjian2664 mentioned this pull request Sep 15, 2024
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 1, 2024

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@chenjian2664 chenjian2664 force-pushed the jdbc_merge_support branch 4 times, most recently from 97870af to 41f8870 Compare December 2, 2024 06:56
chenjian2664
chenjian2664 commented Dec 2, 2024
View reviewed changes
docs/src/main/sphinx/connector/postgresql.md
@@ -115,6 +115,18 @@ catalog named `sales` using the configured connector.
```{include} non-transactional-insert.fragment
```

### Non-transactional MERGE
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mosabua Would you mind to have a look?

@chenjian2664
Copy link
Contributor Author

chenjian2664 commented Dec 2, 2024 •
edited
Loading

I think session property is nice here.

Added the non-transactional-merge session property.

@kokosing
Copy link
Member

kokosing commented Dec 2, 2024

non-transactional-merge-enanbled? Like it is on other session properties? Also see io.trino.plugin.bigquery.BigQuerySessionProperties there _ are used.

@chenjian2664
Copy link
Contributor Author

non-transactional-merge-enanbled? Like it is on other session properties? Also see io.trino.plugin.bigquery.BigQuerySessionProperties there _ are used.

Renamed to non_transactional_merge_enabled

@chenjian2664 chenjian2664 force-pushed the jdbc_merge_support branch 3 times, most recently from c5d7afd to 6870972 Compare December 3, 2024 06:54
@chenjian2664
Support MERGE in base jdbc module
466cb87 
Support MERGE in base jdbc module and refactor Phoenix implementation based on it
@chenjian2664 chenjian2664 requested a review from kokosing December 4, 2024 11:42
kokosing
kokosing approved these changes Dec 6, 2024
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

% comments

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/JdbcWriteConfig.java Show resolved Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcWriteSessionProperties.java Show resolved Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java Show resolved Hide resolved
@chenjian2664 @kokosing
Support merge in Postgresql connector
fb63d83 
Co-Authored-By: Grzegorz Kokosiński <[email protected]>
@kokosing kokosing merged commit e88e2b1 into trinodb:master Dec 9, 2024
95 checks passed
@kokosing
Copy link
Member

kokosing commented Dec 9, 2024

Thank you! Merged.

@github-actions github-actions bot added this to the 468 milestone Dec 9, 2024
@mosabua
Copy link
Member

mosabua commented Dec 9, 2024

Woooohhoooo.. congratulations @chenjian2664 -- excellent work. Also thank you so much for your help and guidance @kokosing

@mosabua mosabua mentioned this pull request Dec 9, 2024
Merged
@chenjian2664 chenjian2664 deleted the jdbc_merge_support branch December 11, 2024 02:54
@shohamyamin
Copy link
Contributor

@chenjian2664 this PR covers support MERGE for oracle as well?

@chenjian2664
Copy link
Contributor Author

@shohamyamin This pr not covers oracle, but I am working on it

@chenjian2664
Copy link
Contributor Author

The oracle work is tracking in #23034 @shohamyamin FYI

@mosabua mosabua mentioned this pull request Dec 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebyhr ebyhr ebyhr left review comments

@losipiuk losipiuk losipiuk left review comments

@kokosing kokosing kokosing approved these changes

@hashhar hashhar Awaiting requested review from hashhar

Assignees

@chenjian2664 chenjian2664

Labels
cla-signed docs hive Hive connector
Milestone
468
Development

Successfully merging this pull request may close these issues.
6 participants
@chenjian2664 @kokosing @mosabua @shohamyamin @losipiuk @ebyhr