[Improvement] jdbc support dialect.name to choose dialect. #7294

liunaijie · 2024-08-01T01:22:33Z

Purpose of this pull request

close #7293

Does this PR introduce any user-facing change?

How was this patch tested?

add new test

Check list

If any new Jar binary package adding in your PR, please add License Notice according
New License Guide
If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs
If you are contributing the connector code, please check that the following files are updated:
1. Update change log that in connector document. For more details you can refer to connector-v2
2. Update plugin-mapping.properties and add new connector information in it
3. Update the pom file of seatunnel-dist
Update the release-note.

Hisoka-X

cc @hailin0

Hisoka-X · 2024-08-02T10:29:00Z

docs/en/connector-v2/source/Jdbc.md

 | table_list                                 | Array   | No       | -               | The list of tables to be read, you can use this configuration instead of `table_path`                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              |
 | where_condition                            | String  | No       | -               | Common row filter conditions for all tables/queries, must start with `where`. for example `where id > 100`                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                         |
 | split.size                                 | Int     | No       | 8096            | How many rows in one split, captured tables are split into multiple splits when read of table.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     |
+| split.hash-function                        | String  | No       | -               | Which hash function use to split. <br/> default is `MD5`, oracle is `ORA_HASH`, Postgres is `HASHTEXT`.<br/>  if the default function is not work, you can use this parameter to customize the hash function.                                                                                                                                                                                                                                                                                                                                                                                                                                      |   |


Suggested change

| split.hash-function | String | No | - | Which hash function use to split. <br/> default is `MD5`, oracle is `ORA_HASH`, Postgres is `HASHTEXT`.<br/> if the default function is not work, you can use this parameter to customize the hash function. | |

| split.hash-function | String | No | - | Which hash function use to split. <br/> Default is `MD5`, Oracle is `ORA_HASH`, PostgreSQL is `HASHTEXT`.<br/> If the default function is not work, you can use this parameter to customize the hash function. | |

Hisoka-X

LGTM.

hailin0 · 2024-08-06T13:51:53Z

Why not separate the dialect from the driver？

cc @Hisoka-X

Jdbc {
    driver = com.mysql.cj.jdbc.Driver
    url = "jdbc:mysql://e2e_starRocksdb:9030"
    
    dialect = "starrocks"
}

liunaijie · 2024-08-07T01:22:38Z

Why not separate the dialect from the driver？

cc @Hisoka-X
Jdbc {
    driver = com.mysql.cj.jdbc.Driver
    url = "jdbc:mysql://e2e_starRocksdb:9030"
    
    dialect = "starrocks"
}

Now the dialect is parse from jdbc url, mysql and starrocks both use mysql driver jdbc:mysql, so will use same mysql dialect.

Hisoka-X · 2024-08-07T05:53:26Z

Why not separate the dialect from the driver？

cc @Hisoka-X
Jdbc {
    driver = com.mysql.cj.jdbc.Driver
    url = "jdbc:mysql://e2e_starRocksdb:9030"
    
    dialect = "starrocks"
}

I prefer adding the split.hash-function parameter because it gives users more options.

hailin0 · 2024-08-07T06:57:06Z

We often use a certain client to access a variety of different database servers, which have many differences. Dialects can cover more differences.

e.g:
mysql client -> doris,starrocks,ocenbase,...
pg client -> ...

Hisoka-X · 2024-08-07T07:00:17Z

We often use a certain client to access a variety of different database servers, which have many differences. Dialects can cover more differences.

e.g: mysql client -> doris,starrocks,ocenbase,... pg client -> ...

I get it. Make sense to me.

liunaijie · 2024-08-08T10:22:27Z

Why not separate the dialect from the driver？

cc @Hisoka-X
Jdbc {
    driver = com.mysql.cj.jdbc.Driver
    url = "jdbc:mysql://e2e_starRocksdb:9030"
    
    dialect = "starrocks"
}

updated, has add a parameter dialect.name, when choose dialect, will both check jdbc url and name.

Hisoka-X · 2024-08-08T12:08:58Z

docs/en/connector-v2/source/Jdbc.md

 | table_list                                 | Array   | No       | -               | The list of tables to be read, you can use this configuration instead of `table_path`                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              |
 | where_condition                            | String  | No       | -               | Common row filter conditions for all tables/queries, must start with `where`. for example `where id > 100`                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                         |
 | split.size                                 | Int     | No       | 8096            | How many rows in one split, captured tables are split into multiple splits when read of table.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     |
+| split.hash-function                        | String  | No       | -               | Which hash function use to split. <br/> Default is `MD5`, Oracle is `ORA_HASH`, PostgreSQL  is `HASHTEXT`.<br/>  If the default function is not work, you can use this parameter to customize the hash function.                                                                                                                                                                                                                                                                                                                                                                                                                                   |   |


Looking back, is it necessary to add this parameter?

i will remove this parameter

hailin0 · 2024-08-09T13:40:21Z

.../apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/starrocks/StarRocksDialect.java

+    }
+
+    @Override
+    public String hashModForField(String functionName, String fieldName, int mod) {


Suggested change

public String hashModForField(String functionName, String fieldName, int mod) {

public String hashModForField(String nativeType, String fieldName, int mod) {

hailin0 · 2024-08-09T13:42:41Z

.../java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/JdbcDialectLoader.java

        final List<JdbcDialectFactory> matchingFactories =
-                foundFactories.stream().filter(f -> f.acceptsURL(url)).collect(Collectors.toList());
+                foundFactories.stream()
+                        .filter(f -> f.acceptsURL(url) && f.getDialectName().equals(dialectName))


Ignore case？

hailin0 · 2024-08-09T13:43:24Z

.../seatunnel/connectors/seatunnel/jdbc/internal/dialect/starrocks/StarRocksDialectFactory.java

+
+    @Override
+    public String getDialectName() {
+        return "starrocks";


Suggested change

return "starrocks";

return DatabaseIdentifier.STARROCKS;

Hisoka-X · 2024-08-12T05:36:57Z

...dbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java

    public static Map<TablePath, JdbcSourceTable> getTables(
-            JdbcConnectionConfig jdbcConnectionConfig, List<JdbcSourceTableConfig> tablesConfig)
+            JdbcConnectionConfig jdbcConnectionConfig,
+            List<JdbcSourceTableConfig> tablesConfig,
+            JdbcSourceConfig sourceConfig)


JdbcSourceConfig sourceConfig

change to

String dialectName

since we only use this parameter.

will update

Hisoka-X · 2024-08-12T05:41:15Z

...or-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/config/JdbcOptions.java

+    Option<String> DIALECT_NAME =
+            Options.key("dialect.name")
+                    .stringType()
+                    .defaultValue("default")
+                    .withDescription(
+                            "The dialect name, when use same jdbc drive, use this parameter to choose the special implement.");


So if I use this config will not work right?

Jdbc { driver = com.mysql.cj.jdbc.Driver url = "jdbc:mysql://mysql:9030" user = root password = "" query = "select * from `test`.`e2e_table_source`" partition_column = "STRING_COL" dialect.name = "mysql" }

It's weird.

yes, if we support dialect.name = "mysql" then this parameter is required to mysql connector, it will impact the current user.

and i think this feature is the special requirement, use same jdbc driver, and the default dialect can not support some feature, it can use this parameter to choose the corresponding dialect implement.

In this case, only those user need this feature need add this parameter

Hisoka-X · 2024-08-14T02:06:36Z

...g/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MySqlDialectFactory.java


    @Override
    public JdbcDialect create(@Nonnull String compatibleMode, String fieldIde) {
+        if (DatabaseIdentifier.STARROCKS.equalsIgnoreCase(compatibleMode)) {


Almost forgot! We have compatibleMode to do such thing.

Hisoka-X

LGTM. Thanks @liunaijie

github-actions bot added document connectors-v2 jdbc e2e labels Aug 1, 2024

liunaijie marked this pull request as ready for review August 2, 2024 01:39

Hisoka-X reviewed Aug 2, 2024

View reviewed changes

Hisoka-X previously approved these changes Aug 6, 2024

View reviewed changes

github-actions bot added approved reviewed labels Aug 6, 2024

liunaijie dismissed Hisoka-X’s stale review via f03d85d August 6, 2024 05:15

github-actions bot removed approved reviewed labels Aug 6, 2024

Hisoka-X previously approved these changes Aug 6, 2024

View reviewed changes

github-actions bot added approved reviewed labels Aug 6, 2024

liunaijie dismissed Hisoka-X’s stale review via 62dbac3 August 8, 2024 10:01

github-actions bot removed approved reviewed labels Aug 8, 2024

Hisoka-X reviewed Aug 8, 2024

View reviewed changes

liunaijie force-pushed the feature/jdbc_split_customize_function branch from e689924 to 214d5f9 Compare August 9, 2024 07:10

hailin0 reviewed Aug 9, 2024

View reviewed changes

Hisoka-X changed the title ~~[Improvement] jdbc split support customize function~~ [Improvement] jdbc support dialect.name to choose dialect. Aug 12, 2024

Hisoka-X reviewed Aug 12, 2024

View reviewed changes

liunaijie force-pushed the feature/jdbc_split_customize_function branch from d4713ea to d5f3ac7 Compare August 13, 2024 06:59

github-actions bot removed the document label Aug 13, 2024

liunaijie force-pushed the feature/jdbc_split_customize_function branch 3 times, most recently from 6b62ed5 to a71f906 Compare August 14, 2024 00:47

Hisoka-X reviewed Aug 14, 2024

View reviewed changes

[Improvement] add starrocks jdbc dialect

25d1b0f

liunaijie force-pushed the feature/jdbc_split_customize_function branch from a71f906 to 25d1b0f Compare August 14, 2024 03:05

github-actions bot added the document label Aug 14, 2024

Hisoka-X approved these changes Aug 14, 2024

View reviewed changes

github-actions bot added approved reviewed labels Aug 14, 2024

hailin0 approved these changes Aug 14, 2024

View reviewed changes

hailin0 merged commit b5140f5 into apache:dev Aug 14, 2024

liunaijie deleted the feature/jdbc_split_customize_function branch August 15, 2024 00:47

liunaijie mentioned this pull request Sep 2, 2024

[Bug] [StarRocks] StarRocks JDBC Dialect confilect with StarRocks Connector #7551

Closed

1 task

	\| split.hash-function \| String \| No \| - \| Which hash function use to split. <br/> default is `MD5`, oracle is `ORA_HASH`, Postgres is `HASHTEXT`.<br/> if the default function is not work, you can use this parameter to customize the hash function. \| \|
	\| split.hash-function \| String \| No \| - \| Which hash function use to split. <br/> Default is `MD5`, Oracle is `ORA_HASH`, PostgreSQL is `HASHTEXT`.<br/> If the default function is not work, you can use this parameter to customize the hash function. \| \|

	public String hashModForField(String functionName, String fieldName, int mod) {
	public String hashModForField(String nativeType, String fieldName, int mod) {

[Improvement] jdbc support dialect.name to choose dialect. #7294

[Improvement] jdbc support dialect.name to choose dialect. #7294

Uh oh!

Conversation

liunaijie commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

Uh oh!

Hisoka-X left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hisoka-X left a comment

Choose a reason for hiding this comment

Uh oh!

hailin0 commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liunaijie commented Aug 7, 2024

Uh oh!

Hisoka-X commented Aug 7, 2024

Uh oh!

hailin0 commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hisoka-X commented Aug 7, 2024

Uh oh!

liunaijie commented Aug 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hisoka-X left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

liunaijie commented Aug 1, 2024 •

edited

Loading

hailin0 commented Aug 6, 2024 •

edited

Loading

hailin0 commented Aug 7, 2024 •

edited

Loading