Skip to content

[Fix][Connector-V2] Fix Paimon branch save mode DDL#10991

Open
QuakeWang wants to merge 1 commit into
apache:devfrom
QuakeWang:paimon-branch-ddl
Open

[Fix][Connector-V2] Fix Paimon branch save mode DDL#10991
QuakeWang wants to merge 1 commit into
apache:devfrom
QuakeWang:paimon-branch-ddl

Conversation

@QuakeWang
Copy link
Copy Markdown
Contributor

Purpose of this pull request

Fix Paimon sink branch handling in save mode and schema evolution.

Previously, Paimon save mode DDL was executed against the main table before the writer switched to the configured branch. This could make schema_save_mode=RECREATE_SCHEMA drop/recreate the main table, data_save_mode=DROP_DATA truncate the main table, or branch auto-create scenarios accidentally create a main table.

Schema evolution also used a non-branch Paimon Identifier, so alter table events were applied to the main table while the writer wrote to the branch.

This PR rejects unsupported non-main branch save mode paths before DDL execution and applies schema evolution with a branch-aware Identifier.

Does this PR introduce any user-facing change?

Yes.

For Paimon sink non-main branches, the main table and target branch must already exist. schema_save_mode=RECREATE_SCHEMA and data_save_mode=DROP_DATA are rejected for non-main branches to avoid modifying the main table.

The Paimon sink documentation is updated with this constraint.

How was this patch tested?

Added PaimonBranchSaveModeHandlerTest to cover:

  • missing main table does not auto-create a main table for branch writes
  • missing branch fails without auto-creating the branch
  • data_save_mode=DROP_DATA does not truncate/drop branch metadata
  • schema_save_mode=RECREATE_SCHEMA does not drop/recreate the main table
  • restore save mode path rejects destructive branch save mode
  • schema evolution alters only the target branch schema

Verified with:

./mvnw spotless:check -pl seatunnel-connectors-v2/connector-paimon
./mvnw -pl seatunnel-connectors-v2/connector-paimon -Dtest=PaimonBranchSaveModeHandlerTest test
./mvnw spotless:apply -pl seatunnel-connectors-v2/connector-paimon
./mvnw -q -DskipTests verify

Check list

Copy link
Copy Markdown
Contributor

@DanielLeens DanielLeens left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I reviewed the latest head from scratch and traced the non-main-branch save-mode path through both the save-mode guard and the schema-evolution handler.

What this PR solves

  • User pain: when writing to a non-main Paimon branch, destructive save modes or schema evolution can accidentally operate on the main table instead of the target branch.
  • Fix approach: fail fast on unsafe branch save modes, require the main table and target branch to exist first, and route branch schema changes to a branch-aware Paimon identifier.
  • One-line summary: the latest head closes the branch/main-table safety gap cleanly, and I did not find a new blocker on this path.

Runtime path I checked

save-mode guard
  -> PaimonSaveModeHandler.handleSchemaSaveMode() [59-72]
      -> checkBranchSaveMode() [91-115]
          -> require main table exists
          -> require target branch exists
          -> reject RECREATE_SCHEMA / DROP_DATA on non-main branches

schema evolution path
  -> PaimonSinkWriter.applySchemaChange(...)
      -> AlterPaimonTableSchemaEventHandler.toIdentifier() [136-143]
          -> uses Identifier(database, table, branch) for non-main branch

test coverage
  -> PaimonBranchSaveModeHandlerTest
      -> destructive branch save modes fail
      -> branch schema change does not mutate the main schema

Review result

  • PaimonSaveModeHandler.java:91-115 now blocks the dangerous cases I was looking for:
    • no implicit main-table auto-create for branch writes,
    • no implicit branch auto-create,
    • no RECREATE_SCHEMA / DROP_DATA on a non-main branch.
  • AlterPaimonTableSchemaEventHandler.java:136-143 is the key correctness change for schema evolution: branch updates now target the branch identifier directly instead of falling back to the main table.
  • PaimonBranchSaveModeHandlerTest.java:92-256 covers the important user-facing cases, including the “branch-only schema change must not touch the main schema” path.
  • I did not find a new source-level blocker on the latest head.

Tests / CI

  • The new tests are targeted and structurally stable.
  • I did not see a flaky-test pattern in the added coverage.
  • The current GitHub Build is green on this head.

Conclusion: can merge

  1. Blocking items
  • None from my side on the latest head.
  1. Suggested non-blocking follow-up
  • None from my side on this path.

From the code-review side this looks ready.

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.

2 participants