-
Notifications
You must be signed in to change notification settings - Fork 2k
[Future][Connector-V2]Support the automatic creation of non-primary key table #9219
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request introduces support for the automatic creation of non-primary key (append-only) tables when the source is a primary key table and fixes the issue of losing primary key information when loading the schema in Paimon Source.
- Adds a new unit test to verify primary key preservation.
- Updates schema conversion and sink configuration to handle the non-primary key scenario.
- Introduces a new error code and configuration option for non-primary key validation.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
seatunnel-connectors-v2/connector-paimon/src/test/java/org/apache/seatunnel/connectors/seatunnel/paimon/catalog/PaimonCatalogPrimaryTest.java | Adds tests to verify primary key behavior. |
seatunnel-connectors-v2/connector-paimon/src/main/java/org/apache/seatunnel/connectors/seatunnel/paimon/utils/SchemaUtil.java | Updates schema conversion to clear primary keys when non-primary key is enabled. |
seatunnel-connectors-v2/connector-paimon/src/main/java/org/apache/seatunnel/connectors/seatunnel/paimon/exception/PaimonConnectorErrorCode.java | Introduces a new error code for non-primary key check errors. |
seatunnel-connectors-v2/connector-paimon/src/main/java/org/apache/seatunnel/connectors/seatunnel/paimon/config/PaimonSinkOptions.java | Adds a new NON_PRIMARY_KEY option with a description that appears ambiguous. |
seatunnel-connectors-v2/connector-paimon/src/main/java/org/apache/seatunnel/connectors/seatunnel/paimon/config/PaimonSinkConfig.java | Implements validation to throw an exception when non-primary key is true but primary keys are provided. |
seatunnel-connectors-v2/connector-paimon/src/main/java/org/apache/seatunnel/connectors/seatunnel/paimon/catalog/PaimonCatalog.java | Adjusts catalog conversion logic and sets primary key and constraint key based on the schema. |
Files not reviewed (1)
- seatunnel-e2e/seatunnel-connector-v2-e2e/connector-paimon-e2e/src/test/resources/changelog_paimon_to_paimon.conf: Language not supported
Comments suppressed due to low confidence (2)
seatunnel-connectors-v2/connector-paimon/src/main/java/org/apache/seatunnel/connectors/seatunnel/paimon/config/PaimonSinkConfig.java:55
- The exception for the non-primary key check is thrown with an empty error message. Provide a descriptive message to aid debugging.
throw new PaimonConnectorException(PaimonConnectorErrorCode.NON_PRIMARY_KEY_CHECK_ERROR, "");
seatunnel-connectors-v2/connector-paimon/src/main/java/org/apache/seatunnel/connectors/seatunnel/paimon/catalog/PaimonCatalog.java:276
- Typo in variable name 'primaryKyes'. Consider renaming it to 'primaryKeys' for clarity.
List<String> primaryKyes = schema.primaryKeys();
@@ -40,6 +40,14 @@ public class PaimonSinkOptions extends PaimonBaseOptions { | |||
.enumType(DataSaveMode.class) | |||
.defaultValue(DataSaveMode.APPEND_DATA) | |||
.withDescription("data_save_mode"); | |||
|
|||
public static final Option<Boolean> NON_PRIMARY_KEY = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description for the NON_PRIMARY_KEY option does not match its functionality (clearing primary keys). Consider updating the description to reflect that setting this option to true disables primary key handling.
Copilot uses AI. Check for mistakes.
List<ConstraintKey.ConstraintKeyColumn> constraintKeyColumns = | ||
primaryKyes.stream() | ||
.map( | ||
pk -> | ||
new ConstraintKey.ConstraintKeyColumn( | ||
pk, ConstraintKey.ColumnSortType.ASC)) | ||
.collect(Collectors.toList()); | ||
builder.constraintKey( | ||
ConstraintKey.of( | ||
ConstraintKey.ConstraintType.UNIQUE_KEY, "uk", constraintKeyColumns)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if a field is a primary key, it must be unique. So do not put it into constraintKey.
...src/main/java/org/apache/seatunnel/connectors/seatunnel/paimon/config/PaimonSinkOptions.java
Outdated
Show resolved
Hide resolved
557c2b3
to
3738339
Compare
3738339
to
bd2c8b6
Compare
Purpose of this pull request
Does this PR introduce any user-facing change?
How was this patch tested?
UT: PaimonCatalogPrimaryTest
E2E: PaimonSinkCDCIT#testChangelogLookup
Check list
New License Guide
release-note
.