Skip to content

feat(datafusion): support the delete_branch procedure - #941

Open
jackylee-ch wants to merge 3 commits into
apache:mainfrom
jackylee-ch:feat/datafusion-delete-branch-procedure
Open

jackylee-ch wants to merge 3 commits into
apache:mainfrom
jackylee-ch:feat/datafusion-delete-branch-procedure

Conversation

@jackylee-ch

Copy link
Copy Markdown
Contributor

Java exposes sys.delete_branch(table, branch) to drop one or more comma-separated branches. The core BranchManager::drop_branch primitive already existed; only the DataFusion CALL procedure was missing.

This registers delete_branch: it declares the table/branch parameters so a misspelled argument is rejected, and dispatches to BranchManager, dropping each named branch and skipping one that no longer exists — the same forgiving, comma-splitting shape as the existing delete_tag.

Tested end-to-end: seed a branch, CALL delete_branch, assert it is gone from $branches; a second delete of the same branch is a no-op.

@JingsongLi

Copy link
Copy Markdown
Contributor

Requirement fit: SUPPORTED, but P1: preserve branches configured for production reads. This procedure calls BranchManager::drop_branch directly. Paimon Java AbstractFileStoreTable.deleteBranch rejects deletion when the name equals scan.primary-branch or scan.fallback-branch; those options protect configured readers. I reproduced the gap at adb3aa5: add WITH ('scan.primary-branch' = 'b1') to the PR integration test table, create b1, then call sys.delete_branch(..., branch => 'b1'). The call succeeds and removes b1. This can break Java or other readers using that configured branch. Please check both table options before any deletion and add regression tests for each, including a comma-separated request where one protected branch appears. The original integration test and formatting check pass; all 14 CI checks are green but do not cover this safety condition. Java reference: https://github.com/apache/paimon/blob/master/paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java

jackylee-ch added a commit to jackylee-ch/paimon-rust that referenced this pull request Sep 26, 2026
Review follow-up (apache#941): `sys.delete_branch` called `BranchManager::drop_branch`
directly, so it would drop a branch named by `scan.primary-branch` or
`scan.fallback-branch` and silently break the table's read path. Paimon Java
`AbstractFileStoreTable.deleteBranch` refuses this and asks the caller to unset
the option first. Add a `BranchManager::ensure_branch_deletable` guard reading
those two options and call it before dropping each branch.
Java exposes `sys.delete_branch(table, branch)` to drop one or more
comma-separated branches. The core BranchManager::drop_branch primitive
already existed; only the DataFusion procedure was missing.

Register delete_branch: declare its table/branch parameters (so a
misspelled argument is rejected, like Java's binding) and dispatch to
BranchManager, dropping each named branch and skipping one that does not
exist -- the same forgiving, comma-splitting shape as delete_tag.
Review follow-up (apache#941): `sys.delete_branch` called `BranchManager::drop_branch`
directly, so it would drop a branch named by `scan.primary-branch` or
`scan.fallback-branch` and silently break the table's read path. Paimon Java
`AbstractFileStoreTable.deleteBranch` refuses this and asks the caller to unset
the option first. Add a `BranchManager::ensure_branch_deletable` guard reading
those two options and call it before dropping each branch.
…ches

Add the SQL regressions requested in review: at the CALL boundary,
`sys.delete_branch` must refuse a branch named by `scan.primary-branch` or
`scan.fallback-branch` and leave it in place, while still deleting unrelated
branches; and a comma-separated request must catch a protected branch even
when it is not listed first.
@jackylee-ch
jackylee-ch force-pushed the feat/datafusion-delete-branch-procedure branch from 1b6706b to 574aa4f Compare September 30, 2026 12:48
@jackylee-ch

Copy link
Copy Markdown
Contributor Author

Addressed. delete_branch now refuses a branch that a reader is configured to consult. proc_delete_branch calls BranchManager::ensure_branch_deletable(table.schema().options(), name) for each branch before dropping it, and that helper checks both scan.primary-branch and scan.fallback-branch, mirroring Java AbstractFileStoreTable.deleteBranch down to the "Unset '…' first" message. Your reproducer now returns the guard error and leaves the configured branch in place instead of removing it.

Added the SQL regressions:

  • test_delete_branch_preserves_scan_configured_branch runs the full CALL path for both option keys: sys.delete_branch on the configured branch fails and the branch survives, while an unrelated branch is still deleted, so the guard is not over-broad.
  • test_delete_branch_batch_stops_at_protected_branch covers a comma-separated request keep,prod where the protected branch is not listed first: the call fails and prod is preserved. Names before the protected one are dropped first, matching Java Table.deleteBranches, which loops deleteBranch per name; the protected branch itself is never dropped.

I verified the tests are non-vacuous: neutering the check makes both fail with "expected error, got Ok" (the protected branch is deleted); restoring it passes. Rebased onto current main. procedures (35 tests) and branch_manager (19 tests, including the two ensure_branch_deletable unit tests) pass; clippy -p paimon -p paimon-datafusion --all-targets -D warnings is clean.

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.

2 participants