JingsongLi commented on PR #939: URL: https://github.com/apache/paimon-rust/pull/939#issuecomment-5936154436
Production review: the ordinary create/duplicate/ignore flows and a real SQL read from a tag-born historical branch work. All 34 existing procedure tests and 18 branch-manager tests passed. Two P2 requirement gaps remain in the new SQL operation: **[P2] Create from a retained tag when its live snapshot has expired** (`proc_create_branch`, `procedures.rs`, around lines 482–485). The delegated core method reads the snapshot from the tag but then unconditionally copies `snapshot/snapshot-<id>` from the main branch. Tags may legitimately outlive that live snapshot JSON. I created three real snapshots, tagged snapshot 1, removed only its live JSON (preserving the tag/schema/manifests/data), and confirmed `SELECT ... VERSION AS OF 'v1'` still returns the original row. `CALL sys.create_branch(..., branch => 'retained', tag => 'v1')` then fails with NotFound for `snapshot-1`, after copying the tag into a partial branch directory. This is a pre-existing core limitation exposed by the new advertised from-tag SQL flow. Please materialize the already-resolved tag snapshot into the new branch when the live snapshot file is absent, as Java `FileSystemBranchManager.createBranch` does. A temporary fallback control creates the branch and reads the original row successfully. **[P2] Reject names that the table reader cannot open** (`BranchManager::validate_branch_name`, `branch_manager.rs`, around lines 70–106). The manager accepts `.`/`..`, but catalog/table branch resolution rejects them (also control characters). `CALL sys.create_branch(..., branch => '..', tag => 'v1')` succeeds and appears in `$branches`; `SELECT * FROM t1$branch_..` immediately fails with `branch name cannot be '.' or '..'`. Please share the catalog's validation before creating metadata, preserving the manager's extra main/numeric restrictions. A temporary shared-validator control rejects the invalid name without creating a branch. Both actual SQL regressions fail on this head; the temporary controls pass them and the normal historical/empty-branch read probe. The earlier path-separator regression is fixed. No production files were touched. -- 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]
