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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
