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.

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

[FLINK-32714] Add dialect for OceanBase database #72

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

whhe
Copy link
Member

@whhe whhe commented Jul 31, 2023

Currently the jdbc url is used to determine the dialect, but OceanBase has two sql compatible modes as 'mysql' and 'oracle', so additional information is needed to determine the sql dialect. In this PR I add a parameter 'compatible-mode', which may break some APIs in the process of creating jdbc dialect.

@boring-cyborg
Copy link

boring-cyborg bot commented Jul 31, 2023

Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html)

@whhe whhe force-pushed the oceanbase-dialect branch 3 times, most recently from 5b24a8f to ede507d Compare August 1, 2023 17:56
@whhe whhe force-pushed the oceanbase-dialect branch 3 times, most recently from f95b4a0 to 29a2307 Compare October 27, 2023 06:43
@whhe whhe marked this pull request as draft October 27, 2023 09:48
@whhe whhe force-pushed the oceanbase-dialect branch 2 times, most recently from 5c39fb2 to d531bb0 Compare October 29, 2023 15:56
@whhe whhe marked this pull request as ready for review October 29, 2023 17:31
@whhe
Copy link
Member Author

whhe commented Oct 30, 2023

I enabled the it cases for Mysql mode of OceanBase, and the ci generally passed. PTAL @leonardBang @MartijnVisser

@MartijnVisser
Copy link
Contributor

@eskabetxe Do you want to take a look?

Copy link
Member

@eskabetxe eskabetxe left a comment

Choose a reason for hiding this comment

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

Hi @whhe,
Thanks for your contribution, I left some comments.

@whhe
Copy link
Member Author

whhe commented Jan 11, 2024

I added test cases for OceanBase Oracle mode, but because there is no docker environment, they can only be executed locally.

Below is the output of testing after I set the connection properties in OceanBaseTestDatabase and remove @Disabled in the test classes.
image

@whhe
Copy link
Member Author

whhe commented Jan 11, 2024

Hello @MartijnVisser @davidradl Is it OK to keep the driver with 'provide' or 'test' scope here? If not, I may have to remove it and disable other IT cases in order to make the CI to pass.

@whhe whhe force-pushed the oceanbase-dialect branch 2 times, most recently from dcb5f3d to f8281c6 Compare January 11, 2024 11:02
@davidradl
Copy link
Contributor

Hello @MartijnVisser @davidradl Is it OK to keep the driver with 'provide' or 'test' scope here? If not, I may have to remove it and disable other IT cases in order to make the CI to pass.

My thinking is that the CI tests should always pass. It think it is ok to leave test code in the codebase that can be used for local tests - ideally with instructions, but they should not case the CI to fail.

@MartijnVisser
Copy link
Contributor

Is it OK to keep the driver with 'provide' or 'test' scope here?

Yes. If we have an enforcer rule that checks if we're not bundling this driver, I think we're safe.

@whhe whhe force-pushed the oceanbase-dialect branch 2 times, most recently from 9467c6e to 9fccfb1 Compare January 12, 2024 06:25
@whhe whhe requested a review from eskabetxe January 12, 2024 07:03
@whhe
Copy link
Member Author

whhe commented Jan 31, 2024

@eskabetxe PTAL

@whhe
Copy link
Member Author

whhe commented Feb 26, 2024

Hi @MartijnVisser @eskabetxe can we continue the review when you have time?

@whhe
Copy link
Member Author

whhe commented Feb 29, 2024

Anyone available please take a look on this.

Copy link
Member

@eskabetxe eskabetxe left a comment

Choose a reason for hiding this comment

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

LGTM

@whhe whhe requested a review from MartijnVisser March 1, 2024 03:02
Copy link
Contributor

@MartijnVisser MartijnVisser left a comment

Choose a reason for hiding this comment

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

LGTM

@MartijnVisser MartijnVisser merged commit 786bd15 into apache:main Mar 1, 2024
12 checks passed
Copy link

boring-cyborg bot commented Mar 1, 2024

Awesome work, congrats on your first merged pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants