-
Notifications
You must be signed in to change notification settings - Fork 407
[lake/paimon] Prohibit users from setting Paimon properties that Fluss depends on #1826
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
[lake/paimon] Prohibit users from setting Paimon properties that Fluss depends on #1826
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 PR prevents users from manually setting Paimon options that Fluss manages, centralizing validation and conversion logic in PaimonConversions and adding tests to enforce the restriction.
- Add validation to block unsettable/default Paimon options and set sane defaults in PaimonConversions.
- Refactor schema and option conversion from PaimonLakeCatalog into PaimonConversions.
- Add an integration test asserting that attempts to set reserved Paimon options throw InvalidConfigException.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| fluss-lake/fluss-lake-paimon/src/main/java/org/apache/fluss/lake/paimon/utils/PaimonConversions.java | Introduces validation for unsettable Paimon options, default option handling, and consolidates conversion utilities. |
| fluss-lake/fluss-lake-paimon/src/main/java/org/apache/fluss/lake/paimon/PaimonLakeCatalog.java | Refactors to use PaimonConversions for identifier/schema/option conversions; removes duplicated logic. |
| fluss-lake/fluss-lake-paimon/src/test/java/org/apache/fluss/lake/paimon/LakeEnabledTableCreateITCase.java | Adds a test ensuring reserved Paimon options cannot be set by users. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| Set<String> keys = new HashSet<>(); | ||
| keys.addAll(PAIMON_UNSETTABLE_OPTIONS); | ||
| keys.addAll(PAIMON_DEFAULT_OPTIONS.keySet()); |
Copilot
AI
Oct 17, 2025
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.
PAIMON_DEFAULT_OPTIONS is an Options instance which does not expose keySet(); this will not compile. Use PAIMON_DEFAULT_OPTIONS.toMap().keySet() to obtain the keys.
| keys.addAll(PAIMON_DEFAULT_OPTIONS.keySet()); | |
| keys.addAll(PAIMON_DEFAULT_OPTIONS.toMap().keySet()); |
...ke/fluss-lake-paimon/src/main/java/org/apache/fluss/lake/paimon/utils/PaimonConversions.java
Outdated
Show resolved
Hide resolved
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| import static org.apache.fluss.lake.paimon.PaimonLakeCatalog.SYSTEM_COLUMNS; |
Copilot
AI
Oct 17, 2025
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.
PaimonConversions now statically depends on PaimonLakeCatalog via SYSTEM_COLUMNS while PaimonLakeCatalog depends on PaimonConversions, creating tight coupling. Consider moving SYSTEM_COLUMNS to a dedicated constants/util class to avoid circular dependencies and improve modularity.
| import static org.apache.fluss.lake.paimon.PaimonLakeCatalog.SYSTEM_COLUMNS; | |
| import static org.apache.fluss.lake.paimon.PaimonLakeConstants.SYSTEM_COLUMNS; |
...ke/fluss-lake-paimon/src/main/java/org/apache/fluss/lake/paimon/utils/PaimonConversions.java
Outdated
Show resolved
Hide resolved
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.
@LiebingYu Thanks for the pr. Left minor comments. PTAL
...ke/fluss-lake-paimon/src/main/java/org/apache/fluss/lake/paimon/utils/PaimonConversions.java
Outdated
Show resolved
Hide resolved
...ke/fluss-lake-paimon/src/main/java/org/apache/fluss/lake/paimon/utils/PaimonConversions.java
Outdated
Show resolved
Hide resolved
...ke/fluss-lake-paimon/src/main/java/org/apache/fluss/lake/paimon/utils/PaimonConversions.java
Show resolved
Hide resolved
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.
+1
Purpose
Linked issue: close #1824
Brief change log
Tests
API and Format
Documentation