You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "jackylee-ch (via GitHub)" <gi...@apache.org> on 2026/09/24 11:57:52 UTC
[PR] feat(datafusion): support the rename_branch procedure [paimon-rust]
jackylee-ch opened a new pull request, #942:
URL: https://github.com/apache/paimon-rust/pull/942
Java exposes `sys.rename_branch(table, from_branch, to_branch)` to rename a branch. The core `BranchManager::rename_branch` primitive already existed; only the DataFusion `CALL` procedure was missing.
This registers `rename_branch`: it declares the `table`/`from_branch`/`to_branch` parameters so a misspelled argument is rejected, and dispatches to `BranchManager`, which already validates that the source branch exists, the target name is free, and neither is `main`.
Tested end-to-end: seed a branch, CALL rename_branch, assert the old name is gone from `$branches` and the new one is present; renaming a missing branch errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(datafusion): support the rename_branch procedure [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #942:
URL: https://github.com/apache/paimon-rust/pull/942#issuecomment-5831567120
Requirement fit: SUPPORTED; the SQL procedure provides a real end-to-end branch operation. However, **P1: reject path separators in branch names before exposing rename via SQL.** `BranchManager::validate_branch_name` accepts `foo/bar`. I reproduced this on the PR head: create branches `b2` and `foo`, then run `CALL sys.rename_branch(table => 'test_db.t1', from_branch => 'b2', to_branch => 'foo/bar')`. The call succeeds, the old branch is gone, but `SELECT * FROM t1$branches WHERE branch_name = 'foo/bar'` returns zero rows: the directory was moved under `branch-foo/bar` and is no longer listed as a branch. This can silently hide a branch and its data from normal discovery. Please reject `/` (and `..` path components) at the shared branch-name validation boundary, and add a SQL regression asserting the call fails and the original branch remains visible. The existing normal rename integration test passes; all 14 CI checks are green, but they do not cover this case. I ran the reproduc
er in a disposable local test directory and restored it afterward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(datafusion): support the rename_branch procedure [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #942:
URL: https://github.com/apache/paimon-rust/pull/942#issuecomment-5936050964
The path-separator regression is fixed and the existing 34 SQL procedure tests plus 18 branch-manager tests pass. I also verified a tag-born branch retains its data after an ordinary SQL rename and can be queried via `t1$branch_b2`.
**[P1] Validate the source branch name before moving its directory.** `rename_branch` validates the target but only checks filesystem existence for the source. After creating a tag-born `b1`, `CALL sys.rename_branch(table => 'test_db.t1', from_branch => 'b1/schema', to_branch => 'stolen')` succeeds: the existing `branch-b1/schema` subdirectory is moved to `branch-stolen`, rather than renaming a branch. A fresh `table.copy_with_branch("b1")` then fails because its schema metadata is missing. I reproduced this on the exact head with a native one-row table and real tag/branch files; the regression fails with `original branch readable=false`. The source is a malformed logical branch name, but the new SQL operation forwards it to a pre-existing core path without name validation. Reject path separators/control characters before checking source existence or mutating files. A temporary source-name guard preserves the original readable branch.
**[P2] Reject target names that readers cannot open.** A validation mismatch remains in the newly exposed rename operation (`BranchManager::validate_branch_name`, `crates/paimon/src/table/branch_manager.rs`, around lines 70–106). The manager accepts the literal target `..`, while the catalog's branch-name validator used by `Table::copy_with_branch` and `$branch_...` table resolution rejects both `.` and `..`. Although `branch-..` is physically a single directory segment, it is not a usable branch name through the table API.
Concrete reproduction: create a native table with one row, create tag `v1`, seed `b1` from that tag, then `CALL sys.rename_branch(table => 'test_db.t1', from_branch => 'b1', to_branch => '..')`. The call succeeds and the original branch disappears, but `table.copy_with_branch("..")` fails with `branch name cannot be '.' or '..'`. This moves a readable branch to a name that normal readers cannot open. The core inconsistency predates this wrapper; the new SQL operation needs to enforce the reader's name contract before moving metadata.
Please share the catalog/table name validation at the manager boundary (also rejecting control characters), retaining the manager's additional main/numeric restrictions. My actual rename/read regression fails on this head; a temporary control calling the catalog validator rejects the target before mutation and preserves the original branch and row.
The source traversal probe `b1/../../snapshot` was rejected by the filesystem backend and preserved the main data; I am not reporting that as a finding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(datafusion): support the rename_branch procedure [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi merged PR #942:
URL: https://github.com/apache/paimon-rust/pull/942
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(datafusion): support the rename_branch procedure [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #942:
URL: https://github.com/apache/paimon-rust/pull/942#discussion_r4177418651
##########
crates/integrations/datafusion/src/procedures.rs:
##########
@@ -453,6 +455,23 @@ async fn proc_create_tag(
ok_result(ctx)
}
+async fn proc_rename_branch(
+ ctx: &SessionContext,
+ catalog: &Arc<dyn Catalog>,
+ catalog_name: &str,
+ args: &HashMap<String, String>,
+) -> DFResult<DataFrame> {
+ let table = get_table(catalog, catalog_name, args).await?;
+ let from_branch = require_arg(args, "from_branch")?;
+ let to_branch = require_arg(args, "to_branch")?;
+
+ let bm = BranchManager::new(table.file_io().clone(), table.location().to_string());
Review Comment:
[P2] Reject REST rename before moving catalog-managed branch files
get_table also accepts RESTCatalog tables, but this constructs a filesystem BranchManager regardless of the table RESTEnv. A real HTTP/Parquet probe first reads t1$branch_b1 through REST (authoritative snapshot 7), then CALL rest.sys.rename_branch(..., from_branch => b1, to_branch => b2) returns success and moves the physical directory without any catalog mutation. Fresh SQL reads of b1 fail with Branch b1 does not exist; b2 is rejected by the unchanged catalog with TableNotExist. Java selects CatalogBranchManager for this environment, and RESTCatalog.renameBranch explicitly throws UnsupportedOperationException before moving files. Preserve that contract by refusing REST rename until a catalog-aware operation exists; a temporary early REST guard makes the same preservation probe pass without breaking native rename.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(datafusion): support the rename_branch procedure [paimon-rust]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on PR #942:
URL: https://github.com/apache/paimon-rust/pull/942#issuecomment-5980013629
Thanks for the review — fixed in the latest commit (CI re-running).
`rename_branch` now refuses tables managed by a REST catalog before building any filesystem `BranchManager`, matching Java `RESTCatalog.renameBranch` (which throws `UnsupportedOperationException`). This avoids moving the physical branch directory out from under the authoritative catalog, which previously left both the old and new names unreadable. Native filesystem renames are unchanged. Added a REST-backed test that asserts the refusal and fails if the guard is removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(datafusion): support the rename_branch procedure [paimon-rust]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on PR #942:
URL: https://github.com/apache/paimon-rust/pull/942#issuecomment-5943097373
Both addressed.
**[P1] Source-name validation.** `rename_branch` now validates the *source* name before any filesystem access, so `from_branch => 'b1/schema'` is rejected up front and `branch-b1` is left intact — it no longer moves the inner `schema` directory. Your reproduction now fails at validation instead of orphaning the branch's metadata.
**[P2] Target names a reader cannot open.** `BranchManager::validate_branch_name` now delegates to the catalog's shared `validate_branch_name` (the exact contract `Table::copy_with_branch` and `$branch_...` resolution enforce): it rejects blank, `.`/`..`, path separators and control characters, and then keeps the manager's extra main/numeric restrictions. So `to_branch => '..'` is rejected before any move, rather than renaming a branch to a name the table API cannot open.
Regression `test_rename_branch_rejects_unopenable_source_and_target`: after seeding a `b1`, both `from_branch => 'b1/schema'` and `to_branch => '..'` fail, `b1` stays listed in `$branches`, and `table.copy_with_branch("b1")` still opens it. Unit coverage `test_validate_branch_name_rejects_reader_unopenable_names` pins `.`/`..`/control-char rejection at the manager. I verified non-vacuity: dropping the source guard makes the `b1/schema` rename succeed and the test fail; restoring it passes.
Rebased onto current main. `procedures` (35 tests) and `branch_manager` (19 tests) pass; `clippy -p paimon -p paimon-datafusion --all-targets --features fulltext,vortex -D warnings` is clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(datafusion): support the rename_branch procedure [paimon-rust]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on PR #942:
URL: https://github.com/apache/paimon-rust/pull/942#issuecomment-5911086098
Addressed. `BranchManager::validate_branch_name` now rejects a target name containing `/` or `\` at the shared validation boundary, so `rename_branch` (and `create_branch`/`create_branch_from_tag`) fail before any snapshot or schema file is moved and the source branch is left intact. Your reproducer now returns the path-separator error instead of silently relocating the branch under `branch-foo/bar`.
On the `..` path component: rejecting `/` and `\` already precludes it. A branch name is placed at the single path segment `branch-<name>`, so with no separator allowed the name can never introduce a new segment, and `..` can only act as a traversal component when it stands alone between separators. A literal name like `..` maps to the ordinary directory `branch-..`, which stays discoverable and cannot escape `branch/`. I deliberately did not blanket-reject the `..` substring, since that would also reject legitimate names such as `v1..v2`.
Added the SQL regression `test_rename_branch_rejects_path_separator`: after creating `b1`, `CALL sys.rename_branch(table => 'test_db.t1', from_branch => 'b1', to_branch => 'foo/bar')` fails with the path-separator error, `b1` is still listed by `t1$branches`, and no `foo/bar` branch is produced. The guard fires before the filesystem is touched, so it holds whether or not a `foo` parent branch already exists. I verified the test is non-vacuous: neutering the check makes the rename return `Ok` and the test fail with "expected error, got Ok"; restoring it passes.
The `validate_branch_name` hardening is the same one-line boundary shared with #939 and #941; whichever lands first, the others rebase cleanly by dropping the duplicate commit. Rebased onto current main. `procedures` (34 tests) and `branch_manager` (18 tests) pass; `clippy -p paimon -p paimon-datafusion --all-targets -D warnings` is clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org