Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]

[via GitHub]

CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2141327842


##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -178,27 +164,48 @@ impl Transaction {
         }
     }
 
-    /// Remove properties in table.
-    pub fn remove_properties(mut self, keys: Vec<String>) -> Result<Self> {
-        self.apply(
-            vec![TableUpdate::RemoveProperties { removals: keys }],
-            vec![],
-        )?;
-        Ok(self)
-    }
-
     /// Set the location of table
-    pub fn set_location(mut self, location: String) -> Result<Self> {
-        self.apply(vec![TableUpdate::SetLocation { location }], vec![])?;
-        Ok(self)
+    pub fn update_location(&self) -> UpdateLocationAction {
+        UpdateLocationAction::new()
     }
 
     /// Commit transaction.
-    pub async fn commit(self, catalog: &dyn Catalog) -> Result<Table> {
+    pub async fn commit(mut self, catalog: &dyn Catalog) -> Result<Table> {
+        if self.actions.is_empty() && self.updates.is_empty() {
+            // nothing to commit
+            return Ok(self.base_table.clone());
+        }
+
+        self.do_commit(catalog).await
+    }
+
+    async fn do_commit(&mut self, catalog: &dyn Catalog) -> Result<Table> {
+        let base_table_identifier = self.base_table.identifier().to_owned();
+
+        let refreshed = 
catalog.load_table(&base_table_identifier.clone()).await?;
+
+        if self.base_table.metadata() != refreshed.metadata()
+            || self.base_table.metadata_location() != 
refreshed.metadata_location()
+        {
+            // current base is stale, use refreshed as base and re-apply 
transaction actions
+            self.base_table = refreshed.clone();
+        }
+
+        let current_table = self.base_table.clone();
+
+        for action in self.actions.clone() {

Review Comment:
   I think this is a good point that we should avoid vec alloc, and I assume 
you meant `for action in &self.actions`? I tried below: 
   ```
   for action in &self.actions {
               let action = Arc::clone(action);
               let mut action_commit = action.commit(&current_table).await?;
               // apply changes to current_table
               self.apply(            
                   action_commit.take_updates(),
                   action_commit.take_requirements(),
               )?;
           }
   ```
   but `self.apply` will be the problem here because it borrows `self` as 
mutable. Ideally, `self.apply` doesn't need to be mutable once we remove 
`updates` and `requirements` from `Transaction`, and change its logic to only 
apply changes to a `Table`, and then we can actually borrow `&self.actions`. 
   
   I think it's probably better to leave it for now and revisit this when 
removing `updates`+`requirements` from `Transaction`, wdyt?



-- 
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: issues-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org