liurenjie1024 commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2142132348
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -17,26 +17,31 @@
//! This module contains transaction api.
-mod action;
+/// The `ApplyTransactionAction` trait provides an `apply` method
+/// that allows users to apply a transaction action to a `Transaction`.
+pub mod action;
Review Comment:
```suggestion
mod action;
pub use action::*;
```
Usually we use this method to hide internal module layer.
##########
crates/iceberg/src/transaction/action.rs:
##########
@@ -35,6 +37,9 @@ pub type BoxedTransactionAction = Arc<dyn TransactionAction>;
/// to modify the table metadata.
#[async_trait]
pub(crate) trait TransactionAction: Sync + Send {
+ /// Returns the action as [`Any`] so it can be downcast to concrete types
later
+ fn as_any(self: Arc<Self>) -> Arc<dyn Any>;
+
Review Comment:
This could be a provided method.
##########
crates/iceberg/src/transaction/action.rs:
##########
@@ -16,6 +16,8 @@
// under the License.
#![allow(dead_code)]
+
Review Comment:
We could remove the `allow` attribute now?
##########
crates/iceberg/src/transaction/upgrade_format_version.rs:
##########
@@ -0,0 +1,133 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::cmp::Ordering;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+
+use crate::TableUpdate::UpgradeFormatVersion;
+use crate::spec::FormatVersion;
+use crate::table::Table;
+use crate::transaction::action::{ActionCommit, TransactionAction};
+use crate::{Error, ErrorKind, Result, TableUpdate};
+
+/// A transaction action to upgrade a table's format version.
+///
+/// This action is used within a transaction to indicate that the
+/// table's format version should be upgraded to a specified version.
+/// The location remains optional until explicitly set via
[`set_format_version`].
+pub struct UpgradeFormatVersionAction {
Review Comment:
Sorry, I pointed to a wrong place:
https://github.com/apache/iceberg/blob/b0b7a63ebdd45e8e5713a85f4e707a2fae55c44e/core/src/main/java/org/apache/iceberg/TableMetadata.java#L596
##########
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:
Sounds reasonable to me, would you mind to create an issue to track this?
##########
crates/iceberg/src/transaction/update_properties.rs:
##########
@@ -0,0 +1,145 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::collections::{HashMap, HashSet};
+use std::sync::Arc;
+
+use async_trait::async_trait;
+
+use crate::table::Table;
+use crate::transaction::action::{ActionCommit, TransactionAction};
+use crate::{Result, TableUpdate};
+
+/// A transactional action that updates or removes table properties
+///
+/// This action is used to modify key-value pairs in a table's metadata
+/// properties during a transaction. It supports setting new values for
existing keys
+/// or adding new keys, as well as removing existing keys. Each key can only
be updated
+/// or removed in a single action, not both.
+pub struct UpdatePropertiesAction {
+ updates: HashMap<String, String>,
+ removals: HashSet<String>,
+}
+
+impl UpdatePropertiesAction {
+ /// Creates a new [`UpdatePropertiesAction`] with no updates or removals.
+ pub fn new() -> Self {
+ UpdatePropertiesAction {
+ updates: HashMap::default(),
+ removals: HashSet::default(),
+ }
+ }
+
+ /// Adds a key-value pair to the update set of this action.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the key was previously marked for removal.
+ ///
+ /// # Arguments
+ ///
+ /// * `key` - The property key to update.
+ /// * `value` - The new value to associate with the key.
+ ///
+ /// # Returns
+ ///
+ /// The updated [`UpdatePropertiesAction`] with the key-value pair added
to the update set.
+ pub fn set(mut self, key: String, value: String) -> Self {
+ assert!(!self.removals.contains(&key));
Review Comment:
No, I mean we should not use `panic` to handle error. We should return an
`Err`. And we could move this check into `apply` method.
--
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
