Skip to content

Conversation

@LiebingYu
Copy link
Contributor

Purpose

Linked issue: close #1824

Brief change log

Tests

API and Format

Documentation

@luoyuxia luoyuxia requested a review from Copilot October 17, 2025 09:16
Copy link

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.

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());
Copy link

Copilot AI Oct 17, 2025

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.

Suggested change
keys.addAll(PAIMON_DEFAULT_OPTIONS.keySet());
keys.addAll(PAIMON_DEFAULT_OPTIONS.toMap().keySet());

Copilot uses AI. Check for mistakes.
import java.util.Map;
import java.util.Set;

import static org.apache.fluss.lake.paimon.PaimonLakeCatalog.SYSTEM_COLUMNS;
Copy link

Copilot AI Oct 17, 2025

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.

Suggested change
import static org.apache.fluss.lake.paimon.PaimonLakeCatalog.SYSTEM_COLUMNS;
import static org.apache.fluss.lake.paimon.PaimonLakeConstants.SYSTEM_COLUMNS;

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@luoyuxia luoyuxia left a 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

Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

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

+1

@luoyuxia luoyuxia merged commit 9481c19 into apache:main Oct 20, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[lake] The default configuration of the Lake tables that Fluss depends on can be tampered with

2 participants