From df3cb07d45610391da0c9c63d2840b81117475ac Mon Sep 17 00:00:00 2001 From: jackylee-ch Date: Thu, 24 Sep 2026 19:56:58 +0800 Subject: [PATCH 1/4] feat(datafusion): support the rename_branch procedure 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 procedure was missing. Register rename_branch: declare its table/from_branch/to_branch parameters (so a misspelled argument is rejected, like Java's binding) and dispatch to BranchManager, which validates that the source branch exists, the target name is free, and neither is main. --- .../integrations/datafusion/src/procedures.rs | 21 ++++++- .../datafusion/tests/procedures.rs | 62 ++++++++++++++++++- 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/crates/integrations/datafusion/src/procedures.rs b/crates/integrations/datafusion/src/procedures.rs index 369c81d9e..5c6881ec4 100644 --- a/crates/integrations/datafusion/src/procedures.rs +++ b/crates/integrations/datafusion/src/procedures.rs @@ -66,7 +66,7 @@ use paimon::catalog::{Catalog, Identifier, RESTCatalog}; use paimon::lumina::LUMINA_IDENTIFIER; use paimon::spec::Snapshot; use paimon::table::{ - normalize_global_index_type_for_drop, SnapshotManager, Table, TagManager, + normalize_global_index_type_for_drop, BranchManager, SnapshotManager, Table, TagManager, SUPPORTED_GLOBAL_INDEX_TYPES_FOR_DROP, }; use paimon::vindex::is_vindex_index_type; @@ -170,6 +170,7 @@ fn declared_parameters(proc_name: &str) -> Option<&'static [&'static str]> { "rollback_to" => &["table", "snapshot_id", "tag"], "rollback_to_timestamp" => &["table", "timestamp"], "create_tag_from_timestamp" => &["table", "tag", "timestamp"], + "rename_branch" => &["table", "from_branch", "to_branch"], "create_global_index" => &["table", "index_column", "index_type", "options"], // `partitions`/`dry_run` are declared but not yet implemented; they still reach // their own "not supported yet" error rather than being reported as unknown. @@ -283,6 +284,7 @@ pub async fn execute_call( "create_tag_from_timestamp" => { proc_create_tag_from_timestamp(ctx, catalog, catalog_name, &args).await } + "rename_branch" => proc_rename_branch(ctx, catalog, catalog_name, &args).await, "create_global_index" => proc_create_global_index(ctx, catalog, catalog_name, &args).await, "drop_global_index" => proc_drop_global_index(ctx, catalog, catalog_name, &args).await, "create_lumina_index" => proc_create_lumina_index(ctx, catalog, catalog_name, &args).await, @@ -453,6 +455,23 @@ async fn proc_create_tag( ok_result(ctx) } +async fn proc_rename_branch( + ctx: &SessionContext, + catalog: &Arc, + catalog_name: &str, + args: &HashMap, +) -> DFResult { + 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()); + bm.rename_branch(from_branch, to_branch) + .await + .map_err(to_datafusion_error)?; + ok_result(ctx) +} + async fn proc_delete_tag( ctx: &SessionContext, catalog: &Arc, diff --git a/crates/integrations/datafusion/tests/procedures.rs b/crates/integrations/datafusion/tests/procedures.rs index 73e4f143e..a6a304ac7 100644 --- a/crates/integrations/datafusion/tests/procedures.rs +++ b/crates/integrations/datafusion/tests/procedures.rs @@ -17,7 +17,13 @@ mod common; -use common::{assert_sql_error, collect_id_name, exec, row_count, setup_sql_context}; +use common::{ + assert_sql_error, collect_id_name, create_sql_context, create_test_env, exec, row_count, + setup_sql_context, +}; +use paimon::catalog::Identifier; +use paimon::table::BranchManager; +use paimon::Catalog; async fn setup_table_with_snapshots() -> (tempfile::TempDir, paimon_datafusion::SQLContext) { let (tmp, sql_context) = setup_sql_context().await; @@ -85,6 +91,60 @@ async fn test_create_tag_with_snapshot_id() { assert_eq!(count, 1); } +#[tokio::test] +async fn test_rename_branch() { + let (_tmp, catalog) = create_test_env(); + let sql_context = create_sql_context(catalog.clone()).await; + exec(&sql_context, "CREATE SCHEMA paimon.test_db").await; + exec( + &sql_context, + "CREATE TABLE paimon.test_db.t1 (id INT, name VARCHAR(100), PRIMARY KEY (id))", + ) + .await; + exec( + &sql_context, + "INSERT INTO paimon.test_db.t1 VALUES (1, 'alice')", + ) + .await; + + // Seed a branch through the core manager (create_branch is a separate PR). + let table = catalog + .get_table(&Identifier::new("test_db", "t1")) + .await + .unwrap(); + let bm = BranchManager::new(table.file_io().clone(), table.location().to_string()); + bm.create_branch("b1").await.unwrap(); + + exec( + &sql_context, + "CALL sys.rename_branch(table => 'test_db.t1', from_branch => 'b1', to_branch => 'b2')", + ) + .await; + + assert!(!bm.branch_exists("b1").await.unwrap(), "old branch gone"); + assert!(bm.branch_exists("b2").await.unwrap(), "new branch present"); + let old = row_count( + &sql_context, + "SELECT * FROM paimon.test_db.`t1$branches` WHERE branch_name = 'b1'", + ) + .await; + assert_eq!(old, 0); + let new = row_count( + &sql_context, + "SELECT * FROM paimon.test_db.`t1$branches` WHERE branch_name = 'b2'", + ) + .await; + assert_eq!(new, 1); + + // Renaming a branch that does not exist is an error. + assert_sql_error( + &sql_context, + "CALL sys.rename_branch(table => 'test_db.t1', from_branch => 'b1', to_branch => 'b3')", + "doesn't exist", + ) + .await; +} + #[tokio::test] async fn test_create_lumina_index_requires_index_column() { let (_tmp, sql_context) = setup_table_with_snapshots().await; From 8c80c63f60776ddbe8d4e94c95660d33ed0907ce Mon Sep 17 00:00:00 2001 From: jackylee-ch Date: Fri, 25 Sep 2026 23:51:13 +0800 Subject: [PATCH 2/4] fix(table): reject path separators in branch names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up (#942): `BranchManager::validate_branch_name` accepted a rename target containing a path separator such as `foo/bar`, so `sys.rename_branch` moved the branch to a nested path (`branch-foo/bar`) that `$branches` never lists back — a silently orphaned branch. Reject '/' and '\\' in branch names, alongside the existing main/blank/numeric checks. --- crates/paimon/src/table/branch_manager.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/paimon/src/table/branch_manager.rs b/crates/paimon/src/table/branch_manager.rs index bd94cdb15..b969c14af 100644 --- a/crates/paimon/src/table/branch_manager.rs +++ b/crates/paimon/src/table/branch_manager.rs @@ -93,6 +93,17 @@ impl BranchManager { source: None, }); } + // A path separator would place the branch directory at a nested path + // (`branch-b1/hidden`), so the branch is created but never listed back by + // `$branches`, leaving it silently orphaned. Reject it up front. + if branch_name.contains('/') || branch_name.contains('\\') { + return Err(crate::Error::DataInvalid { + message: format!( + "Branch name '{branch_name}' must not contain a path separator ('/' or '\\')." + ), + source: None, + }); + } Ok(()) } @@ -409,6 +420,17 @@ mod tests { assert!(BranchManager::validate_branch_name("branch-1").is_ok()); } + #[tokio::test] + async fn test_validate_branch_name_rejects_path_separator() { + // A '/'-bearing rename target creates an unlistable, orphaned branch. + for name in ["b1/hidden", "a\\b"] { + let result = BranchManager::validate_branch_name(name); + assert!(result.is_err(), "'{name}' should be rejected"); + let msg = format!("{}", result.unwrap_err()); + assert!(msg.contains("path separator"), "got: {msg}"); + } + } + #[tokio::test] async fn test_create_branch() { let file_io = test_file_io(); From 7dd9faf52d001333b0fd4f56dd1003770b48400d Mon Sep 17 00:00:00 2001 From: jackylee-ch Date: Wed, 30 Sep 2026 20:01:45 +0800 Subject: [PATCH 3/4] test(datafusion): cover rename_branch rejecting a path-separated name Mirror the create_branch regression for rename: after creating `b1`, `CALL sys.rename_branch(..., to_branch => 'foo/bar')` must fail with the path-separator error, leave `b1` in place (still listed by `$branches`), and not produce a `foo/bar` branch. --- .../datafusion/tests/procedures.rs | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/crates/integrations/datafusion/tests/procedures.rs b/crates/integrations/datafusion/tests/procedures.rs index a6a304ac7..2978a2eea 100644 --- a/crates/integrations/datafusion/tests/procedures.rs +++ b/crates/integrations/datafusion/tests/procedures.rs @@ -145,6 +145,51 @@ async fn test_rename_branch() { .await; } +#[tokio::test] +async fn test_rename_branch_rejects_path_separator() { + let (_tmp, catalog) = create_test_env(); + let sql_context = create_sql_context(catalog.clone()).await; + exec(&sql_context, "CREATE SCHEMA paimon.test_db").await; + exec( + &sql_context, + "CREATE TABLE paimon.test_db.t1 (id INT, name VARCHAR(100), PRIMARY KEY (id))", + ) + .await; + exec( + &sql_context, + "INSERT INTO paimon.test_db.t1 VALUES (1, 'alice')", + ) + .await; + + let table = catalog + .get_table(&Identifier::new("test_db", "t1")) + .await + .unwrap(); + let bm = BranchManager::new(table.file_io().clone(), table.location().to_string()); + bm.create_branch("b1").await.unwrap(); + + // Renaming to `foo/bar` would move the branch under `branch-foo/` and hide it + // from `$branches`; it must be rejected and leave `b1` untouched. + assert_sql_error( + &sql_context, + "CALL sys.rename_branch(table => 'test_db.t1', from_branch => 'b1', to_branch => 'foo/bar')", + "path separator", + ) + .await; + + assert!(bm.branch_exists("b1").await.unwrap(), "b1 must remain"); + assert!( + !bm.branch_exists("foo/bar").await.unwrap(), + "foo/bar must not exist" + ); + let visible = row_count( + &sql_context, + "SELECT * FROM paimon.test_db.`t1$branches` WHERE branch_name = 'b1'", + ) + .await; + assert_eq!(visible, 1); +} + #[tokio::test] async fn test_create_lumina_index_requires_index_column() { let (_tmp, sql_context) = setup_table_with_snapshots().await; From 86074538a26af2ed42cdd9176054fa49035cc02a Mon Sep 17 00:00:00 2001 From: jackylee-ch Date: Fri, 2 Oct 2026 07:56:37 +0800 Subject: [PATCH 4/4] fix(table): validate branch names against the reader contract `BranchManager::validate_branch_name` only rejected the main/numeric/blank cases (and, on this branch, path separators), while the catalog/table reader (`copy_with_branch`, `$branch_...` resolution) also rejects `.`, `..` and control characters. A name accepted by the manager but not the reader lets rename move metadata under a directory no reader can open. Delegate to the shared catalog `validate_branch_name` so the manager enforces exactly the reader's contract, then keep the manager's extra main/numeric rules. Also validate the rename source before touching the filesystem: a malformed `b1/schema` source otherwise renames an inner directory rather than the branch, orphaning its metadata. --- .../datafusion/tests/procedures.rs | 53 +++++++++++++++++++ crates/paimon/src/table/branch_manager.rs | 42 ++++++++------- 2 files changed, 77 insertions(+), 18 deletions(-) diff --git a/crates/integrations/datafusion/tests/procedures.rs b/crates/integrations/datafusion/tests/procedures.rs index 2978a2eea..666b924a0 100644 --- a/crates/integrations/datafusion/tests/procedures.rs +++ b/crates/integrations/datafusion/tests/procedures.rs @@ -190,6 +190,59 @@ async fn test_rename_branch_rejects_path_separator() { assert_eq!(visible, 1); } +#[tokio::test] +async fn test_rename_branch_rejects_unopenable_source_and_target() { + let (_tmp, catalog) = create_test_env(); + let sql_context = create_sql_context(catalog.clone()).await; + exec(&sql_context, "CREATE SCHEMA paimon.test_db").await; + exec( + &sql_context, + "CREATE TABLE paimon.test_db.t1 (id INT, name VARCHAR(100), PRIMARY KEY (id))", + ) + .await; + exec( + &sql_context, + "INSERT INTO paimon.test_db.t1 VALUES (1, 'alice')", + ) + .await; + + let table = catalog + .get_table(&Identifier::new("test_db", "t1")) + .await + .unwrap(); + let bm = BranchManager::new(table.file_io().clone(), table.location().to_string()); + bm.create_branch("b1").await.unwrap(); + + // A path-separated SOURCE would rename `branch-b1/schema` (an inner directory), + // not the branch, orphaning b1's metadata. It must be rejected before any move. + assert_sql_error( + &sql_context, + "CALL sys.rename_branch(table => 'test_db.t1', from_branch => 'b1/schema', to_branch => 'stolen')", + "path separator", + ) + .await; + assert!(bm.branch_exists("b1").await.unwrap(), "b1 must remain"); + assert!( + !bm.branch_exists("stolen").await.unwrap(), + "stolen must not exist" + ); + + // A `..` TARGET is a single directory segment but no reader can open it, so the + // rename must be rejected rather than moving b1 to an unopenable name. + assert_sql_error( + &sql_context, + "CALL sys.rename_branch(table => 'test_db.t1', from_branch => 'b1', to_branch => '..')", + "'.' or '..'", + ) + .await; + assert!( + bm.branch_exists("b1").await.unwrap(), + "b1 must survive a rejected rename" + ); + // b1 is still openable through the table reader. + table.copy_with_branch("b1").await.unwrap(); +} + #[tokio::test] async fn test_create_lumina_index_requires_index_column() { let (_tmp, sql_context) = setup_table_with_snapshots().await; diff --git a/crates/paimon/src/table/branch_manager.rs b/crates/paimon/src/table/branch_manager.rs index b969c14af..489e224e5 100644 --- a/crates/paimon/src/table/branch_manager.rs +++ b/crates/paimon/src/table/branch_manager.rs @@ -69,6 +69,12 @@ impl BranchManager { /// - Cannot be blank or whitespace only /// - Cannot be a pure numeric string fn validate_branch_name(branch_name: &str) -> crate::Result<()> { + // Enforce the same name contract the catalog/table reader applies + // (`copy_with_branch` and `$branch_...` resolution): reject blank, + // `.`/`..`, path separators and control characters. A name accepted here + // must be openable by a reader; otherwise create/rename/delete would move + // metadata under a directory the table API can never resolve. + crate::catalog::validate_branch_name(branch_name)?; if branch_name == DEFAULT_MAIN_BRANCH { return Err(crate::Error::DataInvalid { message: format!( @@ -78,12 +84,6 @@ impl BranchManager { source: None, }); } - if branch_name.trim().is_empty() { - return Err(crate::Error::DataInvalid { - message: format!("Branch name '{}' is blank.", branch_name), - source: None, - }); - } if branch_name.chars().all(|c| c.is_ascii_digit()) { return Err(crate::Error::DataInvalid { message: format!( @@ -93,17 +93,6 @@ impl BranchManager { source: None, }); } - // A path separator would place the branch directory at a nested path - // (`branch-b1/hidden`), so the branch is created but never listed back by - // `$branches`, leaving it silently orphaned. Reject it up front. - if branch_name.contains('/') || branch_name.contains('\\') { - return Err(crate::Error::DataInvalid { - message: format!( - "Branch name '{branch_name}' must not contain a path separator ('/' or '\\')." - ), - source: None, - }); - } Ok(()) } @@ -222,6 +211,10 @@ impl BranchManager { source: None, }); } + // Validate the source name before touching the filesystem: a malformed + // logical name like `b1/schema` would otherwise rename a branch's inner + // directory, not the branch, orphaning its metadata. + Self::validate_branch_name(from)?; if !self.branch_exists(from).await? { return Err(crate::Error::DataInvalid { message: format!("Branch name '{}' doesn't exist.", from), @@ -403,7 +396,7 @@ mod tests { let result = BranchManager::validate_branch_name(""); assert!(result.is_err()); let msg = format!("{}", result.unwrap_err()); - assert!(msg.contains("blank")); + assert!(msg.contains("empty"), "got: {msg}"); } #[tokio::test] @@ -431,6 +424,19 @@ mod tests { } } + #[tokio::test] + async fn test_validate_branch_name_rejects_reader_unopenable_names() { + // Names the catalog/table reader rejects (`.`, `..`, control chars) must + // also be rejected here, so a created/renamed branch is always openable + // via `copy_with_branch` / `$branch_...`. + for name in [".", "..", "a\u{0007}b", "a\u{001C}b"] { + assert!( + BranchManager::validate_branch_name(name).is_err(), + "{name:?} should be rejected" + ); + } + } + #[tokio::test] async fn test_create_branch() { let file_io = test_file_io();