jackylee-ch commented on code in PR #941:
URL: https://github.com/apache/paimon-rust/pull/941#discussion_r4282767286
##########
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:
Fixed in af28136274d5f8bf8e750ddfc75446ec2c2f8dbd. `drop_branch` now deletes
the `branch-<name>/` form that `branch_exists` checks, so OpenDAL's path trim
can no longer turn `prod ` into `prod`; the slash also bounds the prefix
listing of a non-native recursive delete, where dropping `b1` took `b10`. Added
your SQL regression (tag-born `prod` and `prod `; `prod` stays readable) and a
core prefix test; both fail without the fix.
--
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]
