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

[Feature][Jdbc] Add Jdbc default dialect for all jdbc series database without dialect #8132

Merged
merged 1 commit into from
Nov 26, 2024
Merged

[Feature][Jdbc] Add Jdbc default dialect for all jdbc series database without dialect #8132

merged 1 commit into from
Nov 26, 2024

Conversation

sunxiaojian
Copy link
Contributor

@sunxiaojian sunxiaojian commented Nov 24, 2024 •
edited
Loading

Purpose of this pull request

closed : #7956

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@github-actions github-actions bot removed the Zeta label Nov 24, 2024
@sunxiaojian sunxiaojian force-pushed the improve-dialect branch 5 times, most recently from 90dd7f7 to eedb034 Compare November 24, 2024 14:47
@sunxiaojian
Copy link
Contributor Author

@Hisoka-X PTAL, The CI error should not be related to this modification

@Hisoka-X
Copy link
Member

@Hisoka-X PTAL, The CI error should not be related to this modification

Please rebase from dev. The error already fixed in #8110

@sunxiaojian
Copy link
Contributor Author

sunxiaojian commented Nov 25, 2024 •
edited
Loading

@Hisoka-X PTAL, The CI error should not be related to this modification

Please rebase from dev. The error already fixed in #8110

@Hisoka-X done， PTAL

Hisoka-X
Hisoka-X reviewed Nov 25, 2024
View reviewed changes
Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

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

Thanks @sunxiaojian !

...ctor-jdbc-e2e/connector-jdbc-e2e-part-1/src/test/resources/jdbc_mariadb_source_and_sink.conf
user = "mariadb_user"
password = "mariadb_password"

query = "select * from source;"
Copy link
Member

@Hisoka-X Hisoka-X Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the generic dialect should support table_path and catalog table too.

Copy link
Member

@Hisoka-X Hisoka-X Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

oh. It aleady supported by JdbcCatalogUtils. Please create a test case with only table_path.

Copy link
Contributor Author

@sunxiaojian sunxiaojian Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@Hisoka-X done， PTAL

@github-actions github-actions bot added the api label Nov 25, 2024
Hisoka-X
Hisoka-X reviewed Nov 26, 2024
View reviewed changes
...c-e2e-part-1/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMariaDBIT.java
private static final String MARIADB_SINK = "sink";
private static final String CATALOG_DATABASE = "catalog_database";

private static final List<String> CONFIG_FILE =
Copy link
Member

Choose a reason for hiding this comment

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

Please merge two test case into one class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this necessary？

Copy link
Member

Choose a reason for hiding this comment

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

To save ci time and remove duplicated code.

seatunnel-api/src/main/java/org/apache/seatunnel/api/table/converter/BasicTypeDefine.java
@@ -31,6 +31,7 @@ public class BasicTypeDefine<T> implements Serializable {
protected String columnType;
// e.g. `varchar` for MySQL
protected String dataType;
protected int sqlType;
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use nativeType instead of sqlType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can use nativeType instead of sqlType.

no, like 'INT' cannot be converted to standard JDBCType, SQLType doesn't need to worry about this issue

Copy link
Member

Choose a reason for hiding this comment

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

OK, please add some comment to tell devs the sqlType come from jdbc, not come from seatunnel sql type. cc @hailin0

Hisoka-X
Hisoka-X approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

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

LGTM if ci passes. Thanks @sunxiaojian

@hailin0 hailin0 merged commit 399eabc into apache:dev Nov 26, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hisoka-X Hisoka-X Hisoka-X approved these changes

@hailin0 hailin0 hailin0 approved these changes

Assignees
No one assigned
Labels
api approved connectors-v2 e2e jdbc reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature][Jdbc] Add Jdbc default dialect for all jdbc series database without dialect
3 participants
@sunxiaojian @Hisoka-X @hailin0