Skip to content

Conversation

@hailin0
Copy link
Member

@hailin0 hailin0 commented Apr 3, 2025

Purpose of this pull request

[Transform] Support define sink column type

Does this PR introduce any user-facing change?

add new transform

How was this patch tested?

Check list

@hailin0 hailin0 force-pushed the dev-transform-sinktype branch from 4a7ab15 to a08a605 Compare April 7, 2025 04:10
@github-actions github-actions bot added the core SeaTunnel core module label Apr 7, 2025
@hailin0 hailin0 force-pushed the dev-transform-sinktype branch from a08a605 to dfad0e1 Compare April 7, 2025 09:12
@hailin0 hailin0 force-pushed the dev-transform-sinktype branch 2 times, most recently from 225ec7b to ea5708a Compare April 8, 2025 05:02
@hailin0 hailin0 requested a review from Copilot April 9, 2025 10:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 45 out of 45 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/iris/IrisCreateTableSqlBuilder.java:113

  • Consider using a check that also validates for blank values (e.g. StringUtils.isNotBlank(column.getSinkType())) instead of only a null check to avoid unintended behavior when an empty string is provided.
if (column.getSinkType() != null) {

seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/mysql/MysqlCreateTableSqlBuilder.java:182

  • [nitpick] Consider specifying an explicit access modifier (e.g. private) for the buildColumnIdentifySql method to ensure consistency and clarity with similar methods in other catalog builder classes.
String buildColumnIdentifySql(

@hailin0 hailin0 marked this pull request as ready for review April 9, 2025 10:11
@hailin0
Copy link
Member Author

hailin0 commented Apr 9, 2025

PTAL

@hailin0 hailin0 requested a review from Copilot April 10, 2025 03:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 45 out of 45 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

seatunnel-connectors-v2/connector-jdbc/catalog/sqlserver/SqlServerCreateTableSqlBuilder.java:199

  • [nitpick] Consider using a check for a non-blank sink type (e.g., StringUtils.isNotBlank(column.getSinkType())) instead of only a null check to avoid using empty strings as column types.
if (column.getSinkType() != null) {

seatunnel-connectors-v2/connector-jdbc/catalog/psql/PostgresCreateTableSqlBuilder.java:127

  • The access modifier for buildColumnSql was changed from private to package-private; if this change is not intentional, consider reverting to private to preserve encapsulation.
String buildColumnSql(Column column) {

seatunnel-connectors-v2/connector-clickhouse/util/ClickhouseCatalogUtil.java:35

  • [nitpick] Consider validating that column.getSinkType() is not only non-null but also non-empty to prevent empty string values from being used as the column type.
if (column.getSinkType() != null) {

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

public class DefineSinkTypeTransformTest {
Copy link
Member

Choose a reason for hiding this comment

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

Test with multi-table?

Copy link
Member Author

@hailin0 hailin0 Apr 19, 2025

Choose a reason for hiding this comment

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

@Hisoka-X fixed

@hailin0 hailin0 force-pushed the dev-transform-sinktype branch from ea5708a to e891ecf Compare April 17, 2025 15:54
@hailin0 hailin0 force-pushed the dev-transform-sinktype branch from e891ecf to 155dd67 Compare April 17, 2025 17:06
@corgy-w corgy-w merged commit ab7119e into apache:dev Apr 22, 2025
6 checks passed
hawk9821 pushed a commit to hawk9821/seatunnel that referenced this pull request Apr 22, 2025
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.

3 participants