JingsongLi commented on code in PR #482:
URL: https://github.com/apache/paimon-rust/pull/482#discussion_r3544443320
##########
crates/paimon/src/table/mod.rs:
##########
@@ -302,6 +342,44 @@ impl Table {
Ok(table)
}
+ pub async fn copy_with_branch(&self, branch_name: &str) -> Result<Self> {
+ let branch = if branch_name.trim().is_empty() {
+ return Err(crate::Error::DataInvalid {
+ message: "Branch name cannot be empty.".to_string(),
+ source: None,
+ });
+ } else {
+ validate_branch_name(branch_name)?;
+ branch_name.to_string()
+ };
+ let schema_manager = if branch == DEFAULT_MAIN_BRANCH {
+ SchemaManager::new(self.file_io.clone(), self.location.clone())
+ } else {
+ SchemaManager::new(self.file_io.clone(),
self.location.clone()).with_branch(&branch)
+ };
+ let schema = schema_manager
+ .latest()
+ .await?
+ .ok_or_else(|| crate::Error::DataInvalid {
+ message: format!("Branch '{branch}' does not exist."),
+ source: None,
+ })?;
+ let mut options = schema.options().clone();
+ options.insert("branch".to_string(), branch.clone());
+ Ok(Self {
+ file_io: self.file_io.clone(),
+ identifier: self.identifier.clone(),
+ location: self.location.clone(),
+ schema: schema.copy_with_replaced_options(options),
+ schema_manager,
+ branch,
+ branch_reference: true,
Review Comment:
This marks branch copies as read-only for `WriteBuilder`, but it does not
cover other public mutating builders that bypass `WriteBuilder` and call
`TableCommit::new` directly, such as the global-index build/drop builders. A
user can still call a branch copy's index builder and commit root metadata.
Please centralize the branch-reference write guard in a fallible commit path,
or explicitly reject branch references in every public mutating builder before
exposing branch copies.
##########
crates/integrations/datafusion/src/sql_context.rs:
##########
@@ -1425,6 +1482,24 @@ impl SQLContext {
}
}
+ fn ensure_main_branch_write_target(name: &ObjectName, operation: &str) ->
DFResult<()> {
+ let object = name
+ .0
+ .last()
+ .and_then(|part| part.as_ident())
+ .map(|ident| ident.value.as_str())
+ .ok_or_else(|| DataFusionError::Plan(format!("Invalid table
reference: {name}")))?;
+ let parsed = parse_object_name(object).map_err(to_datafusion_error)?;
+ if let Some(branch) = parsed.branch() {
+ if branch != DEFAULT_MAIN_BRANCH {
Review Comment:
`$branch_main` is still a branch reference, but this guard allows it
through. For intercepted writes (`UPDATE`, `DELETE`, `MERGE`, `TRUNCATE`,
`ALTER`, `INSERT OVERWRITE`), this can fall through to table-not-found or `IF
EXISTS` no-op behavior instead of the documented read-only rejection. Please
reject whenever `parsed.branch().is_some()` and add `$branch_main` coverage for
these write handlers.
--
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]
