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

[Feature][Catalog] Catalog add Case Conversion Definition #5328

Merged
merged 14 commits into from
Sep 12, 2023

Conversation

XiaoJiang521
Copy link
Contributor

Purpose of this pull request

Check list

@Hisoka-X
Copy link
Member

There are so many same String.format for same purpose. Could you refactor it? Please.
image

String randomSuffix = UUID.randomUUID().toString().replace("-", "").substring(0, 4);
String columnNamesString = String.join(", ", primaryKey.getColumnNames());
// String columnNamesString = String.join(", ", primaryKey.getColumnNames());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// String columnNamesString = String.join(", ", primaryKey.getColumnNames());

return build(tablePath, "");
}

public String build(TablePath tablePath, String fieldIde) {
Copy link
Member

Choose a reason for hiding this comment

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

Put fieldIde as PostgresCreateTableSqlBuilder field, so you don't need it as parameter for much methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


package org.apache.seatunnel.connectors.seatunnel.jdbc.internal.dialect.dialectenum;

public enum FieldIdeEnum {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a litttle curious, where did you get the name: FieldIde? Because it just LetterCase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

field identifier😂 I'm not sure if this is acceptable either.

Copy link
Member

Choose a reason for hiding this comment

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

How about IdentifierCase?

@Hisoka-X
Copy link
Member

Is any possible we can create AbstractCreateTableSqlBuilder to define common behavior of OracleCreateTableSqlBuilder, MySQLCreateTableSqlBuilder so on?

@Hisoka-X Hisoka-X added this to the 2.3.4 milestone Aug 24, 2023
@Hisoka-X Hisoka-X changed the title [feature][Catalog] catalog add indent [Feature][Catalog] Catalog add indent Aug 24, 2023
@ic4y ic4y closed this Aug 25, 2023
@ic4y ic4y reopened this Aug 25, 2023
@XiaoJiang521
Copy link
Contributor Author

Is any possible we can create AbstractCreateTableSqlBuilder to define common behavior of OracleCreateTableSqlBuilder, MySQLCreateTableSqlBuilder so on?

ok

@XiaoJiang521
Copy link
Contributor Author

There are so many same String.format for same purpose. Could you refactor it? Please. image

I'm uncertain about how to refactor this part. It can be categorized into two groups: one is "SchemaAndTable," and the other is related to "getFullName." Additionally, I've introduced an overloaded method named "getFullNameWithQuoted" due to cases involving both field and [field]. Could you provide some suggestions for refactoring

@XiaoJiang521
Copy link
Contributor Author

AbstractCreateTableSqlBuilder

The four classes, namely MysqlCreateTableSqlBuilder, OracleCreateTableSqlBuilder, PostgresCreateTableSqlBuilder, and SqlServerCreateTableSqlBuilder, share some commonality only between OracleCreateTableSqlBuilder and PostgresCreateTableSqlBuilder. For the rest, aside from class attributes that can be extracted into an abstract class, there doesn't appear to be much commonality to extract since each database requires its own unique table creation SQL construction and they differ significantly. Would it still be necessary to create an abstract class called AbstractCreateTableSqlBuilder for this situation?

@Hisoka-X
Copy link
Member

Hisoka-X commented Sep 5, 2023

AbstractCreateTableSqlBuilder

The four classes, namely MysqlCreateTableSqlBuilder, OracleCreateTableSqlBuilder, PostgresCreateTableSqlBuilder, and SqlServerCreateTableSqlBuilder, share some commonality only between OracleCreateTableSqlBuilder and PostgresCreateTableSqlBuilder. For the rest, aside from class attributes that can be extracted into an abstract class, there doesn't appear to be much commonality to extract since each database requires its own unique table creation SQL construction and they differ significantly. Would it still be necessary to create an abstract class called AbstractCreateTableSqlBuilder for this situation?

I see, we can do it later after we have catalog module.

@ic4y ic4y closed this Sep 5, 2023
@ic4y ic4y reopened this Sep 5, 2023
@EricJoy2048 EricJoy2048 changed the title [Feature][Catalog] Catalog add indent [Feature][Catalog] Catalog add Case Conversion Definition Sep 11, 2023
EricJoy2048
EricJoy2048 previously approved these changes Sep 12, 2023
@hailin0 hailin0 merged commit 7b5b28b into apache:dev Sep 12, 2023
53 checks passed
gnehil pushed a commit to gnehil/seatunnel that referenced this pull request Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants