JingsongLi commented on code in PR #941:
URL: https://github.com/apache/paimon-rust/pull/941#discussion_r4206021660
##########
crates/integrations/datafusion/src/procedures.rs:
##########
@@ -487,6 +489,58 @@ async fn proc_rename_branch(
ok_result(ctx)
}
+async fn proc_delete_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 branch_str = require_arg(args, "branch")?;
+
+ // A REST catalog owns branch metadata and the Rust REST catalog has no
+ // branch-drop endpoint yet (Java `RESTCatalog.dropBranch` DELETEs through
the
+ // catalog). Dropping the physical branch directory here would leave the
+ // catalog still listing the branch while its data is gone. Refuse before
+ // deleting anything, until a catalog-aware path exists.
+ if table.rest_env().is_some() {
+ return Err(DataFusionError::NotImplemented(
+ "delete_branch is not yet supported for tables managed by a REST
catalog".to_string(),
+ ));
+ }
+
+ let bm = BranchManager::new(table.file_io().clone(),
table.location().to_string());
+ let options = table.schema().options();
+ // Java `Table.deleteBranches` splits on comma and passes each token
through
+ // unchanged. Branch identities keep leading/trailing spaces (the shared
+ // validator and readers treat `" b1"` and `"b1"` as distinct), so the
+ // delete_tag trim convention must not apply here — trimming could delete a
+ // different branch than the one requested.
+ for branch_name in branch_str.split(',') {
+ if branch_name.is_empty() {
+ continue;
+ }
+ // Validate the logical name before any existence check or deletion: a
+ // separator-bearing name like `prod/schema` would otherwise slip past
the
+ // configured-branch guard (it is not literally `prod`) and recursively
+ // delete the inner directory of a protected branch.
+
BranchManager::validate_branch_name(branch_name).map_err(to_datafusion_error)?;
+ BranchManager::ensure_branch_deletable(options, branch_name)
+ .map_err(to_datafusion_error)?;
+ if !bm
+ .branch_exists(branch_name)
+ .await
+ .map_err(to_datafusion_error)?
+ {
+ continue;
+ }
+ bm.drop_branch(branch_name)
Review Comment:
[P1] Preserve trailing-space branch identity in recursive deletion
The new SQL path accepts trailing spaces and checks protection against the
unchanged name, but BranchManager::drop_branch passes branch_path without a
trailing slash to FileIO.delete_dir. OpenDAL normalizes that deletion path with
path.trim(). I reproduced on a real FileSystemCatalog through SQL: configure
scan.primary-branch=prod, create prod and the distinct valid branch "prod "
from the same tag, then CALL sys.delete_branch(table => 'test_db.t1', branch =>
'prod '). The call succeeds, recursively removes protected branch-prod, and
leaves the requested trailing-space branch present. branch_exists had checked
the exact spaced directory because it appends a slash, so it did not catch the
deletion-path mismatch. The raw helper behavior predates the PR, but exposing
it through this guarded procedure lets a request for an unrelated valid branch
delete a configured production branch and its snapshot/schema metadata. Please
pass a directory path ending in / (or otherwise preserve the f
ull branch identity) into deletion, and add a SQL regression that asserts both
the requested branch is removed and protected prod remains readable.
--
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]
