You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "CTTY (via GitHub)" <gi...@apache.org> on 2025/06/10 18:52:44 UTC
[PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
CTTY opened a new pull request, #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Related Issues:
- #1382 [EPIC]
- #1386
- #1387
- #1388
- #1389
## What changes are included in this PR?
<!--
Provide a summary of the modifications in this PR. List the main changes such as new features, bug fixes, refactoring, or any other updates.
-->
- Added implementations of `TransactionAction`
- `UpdateLocationAction`
- `UpdatePropertiesAction`
- `UpgradeFormatVersionAction`
- Added `as_any` to `TransactionAction` trait
## Are these changes tested?
Added unit tests
<!--
Specify what test covers (unit test, integration test, etc.).
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2143455195
##########
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:
No worries! I tried merging this logic with `update_properties` but I still prefer keeping an independent `UpgradeFormatVersionAction`: merging this to `UpdatePropAction` will make `UpdatePropAction::commit` noisier, as it has to check if `updates` contains this property, convert it to `FormatVersion`, and finally append a `TableUpdate` if needed.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2138992976
##########
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:
I added this mainly for testing/inspecting the content of applied `TransactionAction` s
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2145936565
##########
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:
Thanks Renjie, I gave it a try and it's working fine except some syntax sugar to use it on Arc<T>:
```
let action = (*tx.actions[0]) // this
.downcast_ref::<UpdateLocationAction>()
.unwrap();
```
This is still much cleaner than using `Any` directly!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2138994965
##########
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() {
+ let mut action_commit = action.commit(¤t_table).await?;
Review Comment:
This will only commit actions that implement the `TransactionAction` traits. Actions like `FastAppendAction` and `ReplaceSortOrder` will still use the tx.apply() directly for now. I plan to implement them in a separate PR
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2141331593
##########
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:
I think `DEFAULT_FILE_FORMAT` is different that it refers to the formats like parquet, avro, etc.
https://iceberg.apache.org/docs/latest/configuration/#read-properties:~:text=write.format.default
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2145936565
##########
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:
Thanks Renjie, I gave it a try and it's working fine except some syntax sugar to use it on Arc<T>:
```
let action = (&*tx.actions[0]) // this
.downcast_ref::<UpdateLocationAction>()
.unwrap();
```
This is still much cleaner than using `Any` directly!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2148863363
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -104,32 +110,13 @@ impl Transaction {
}
/// Sets table to a new version.
- pub fn upgrade_table_version(mut self, format_version: FormatVersion) -> Result<Self> {
- let current_version = self.current_table.metadata().format_version();
- match current_version.cmp(&format_version) {
- Ordering::Greater => {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Cannot downgrade table version from {} to {}",
- current_version, format_version
- ),
- ));
- }
- Ordering::Less => {
- self.apply(vec![UpgradeFormatVersion { format_version }], vec![])?;
- }
- Ordering::Equal => {
- // Do nothing.
- }
- }
- Ok(self)
+ pub fn upgrade_table_version(&self) -> UpgradeFormatVersionAction {
+ UpgradeFormatVersionAction::new()
}
/// Update table's property.
- pub fn set_properties(mut self, props: HashMap<String, String>) -> Result<Self> {
Review Comment:
The release interval certainly is an [issue](https://github.com/apache/iceberg-rust/issues/1327) (pun intented π )
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2143427969
##########
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:
I played with this a little bit but it seems like this has to be done within concrete implementations. With code like below:
```
fn as_any(self: Arc<Self>) -> Arc<dyn Any> {
self
}
```
And this will fail to the error:
```
|
43 | self
| ^^^^ doesn't have a size known at compile-time
```
It seems that even if self is an `Arc<T>`, the contained `T` still has to be `Sized` to be cast to `Any` in a provided method. Adding `where Self: Sized` bound to the method directly will make it uncallable on `Arc<dyn TxAction>` so it won't work as well
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "dentiny (via GitHub)" <gi...@apache.org>.
dentiny commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2148861528
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -104,32 +110,13 @@ impl Transaction {
}
/// Sets table to a new version.
- pub fn upgrade_table_version(mut self, format_version: FormatVersion) -> Result<Self> {
- let current_version = self.current_table.metadata().format_version();
- match current_version.cmp(&format_version) {
- Ordering::Greater => {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Cannot downgrade table version from {} to {}",
- current_version, format_version
- ),
- ));
- }
- Ordering::Less => {
- self.apply(vec![UpgradeFormatVersion { format_version }], vec![])?;
- }
- Ordering::Equal => {
- // Do nothing.
- }
- }
- Ok(self)
+ pub fn upgrade_table_version(&self) -> UpgradeFormatVersionAction {
+ UpgradeFormatVersionAction::new()
}
/// Update table's property.
- pub fn set_properties(mut self, props: HashMap<String, String>) -> Result<Self> {
Review Comment:
> We definitely need to call this out in the next release notes.
Thanks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2141223957
##########
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:
I'm not sure I understand the difference it makes by moving the condition check to `apply`. if the an user use it like below:
```
let action = tx.update_table_properties
action
.set(k1,v)
.remove(k1) // panic here
.apply(tx) // if the check was moved to apply, then we would panic here
```
I think the effect is basically the same, and checking this condition early on can help users fail early since this error is non-retryable
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "dentiny (via GitHub)" <gi...@apache.org>.
dentiny commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2148861764
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -104,32 +110,13 @@ impl Transaction {
}
/// Sets table to a new version.
- pub fn upgrade_table_version(mut self, format_version: FormatVersion) -> Result<Self> {
- let current_version = self.current_table.metadata().format_version();
- match current_version.cmp(&format_version) {
- Ordering::Greater => {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Cannot downgrade table version from {} to {}",
- current_version, format_version
- ),
- ));
- }
- Ordering::Less => {
- self.apply(vec![UpgradeFormatVersion { format_version }], vec![])?;
- }
- Ordering::Equal => {
- // Do nothing.
- }
- }
- Ok(self)
+ pub fn upgrade_table_version(&self) -> UpgradeFormatVersionAction {
+ UpgradeFormatVersionAction::new()
}
/// Update table's property.
- pub fn set_properties(mut self, props: HashMap<String, String>) -> Result<Self> {
Review Comment:
> I'm not very familiar with moonlink and here goes the dumb question: Would it be better for moonlink to depend on a released/stable iceberg-rs version instead of tracking an unreleased git rev?
That's the ideal case, but I think the interval between iceberg-rust releases are too long.
> You can set up a non-blocking github CI to track iceberg-rs/main if you want to catch compatibility issues early
It's not too much of an overhead as of now, because I sync iceberg-rust upstream on weekly basic.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "dentiny (via GitHub)" <gi...@apache.org>.
dentiny commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2148861764
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -104,32 +110,13 @@ impl Transaction {
}
/// Sets table to a new version.
- pub fn upgrade_table_version(mut self, format_version: FormatVersion) -> Result<Self> {
- let current_version = self.current_table.metadata().format_version();
- match current_version.cmp(&format_version) {
- Ordering::Greater => {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Cannot downgrade table version from {} to {}",
- current_version, format_version
- ),
- ));
- }
- Ordering::Less => {
- self.apply(vec![UpgradeFormatVersion { format_version }], vec![])?;
- }
- Ordering::Equal => {
- // Do nothing.
- }
- }
- Ok(self)
+ pub fn upgrade_table_version(&self) -> UpgradeFormatVersionAction {
+ UpgradeFormatVersionAction::new()
}
/// Update table's property.
- pub fn set_properties(mut self, props: HashMap<String, String>) -> Result<Self> {
Review Comment:
> I'm not very familiar with moonlink and here goes the dumb question: Would it be better for moonlink to depend on a released/stable iceberg-rs version instead of tracking an unreleased git rev?
That's the ideal case, but I think the interval between iceberg-rust releases are too long.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2148863363
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -104,32 +110,13 @@ impl Transaction {
}
/// Sets table to a new version.
- pub fn upgrade_table_version(mut self, format_version: FormatVersion) -> Result<Self> {
- let current_version = self.current_table.metadata().format_version();
- match current_version.cmp(&format_version) {
- Ordering::Greater => {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Cannot downgrade table version from {} to {}",
- current_version, format_version
- ),
- ));
- }
- Ordering::Less => {
- self.apply(vec![UpgradeFormatVersion { format_version }], vec![])?;
- }
- Ordering::Equal => {
- // Do nothing.
- }
- }
- Ok(self)
+ pub fn upgrade_table_version(&self) -> UpgradeFormatVersionAction {
+ UpgradeFormatVersionAction::new()
}
/// Update table's property.
- pub fn set_properties(mut self, props: HashMap<String, String>) -> Result<Self> {
Review Comment:
The release interval certainly is an [issue](https://github.com/apache/iceberg-rust/issues/1327) π
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2143366893
##########
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:
Good point!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2143397457
##########
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:
This makes sense. I've moved this check to `commit`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
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(¤t_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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 merged PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2145494182
##########
crates/iceberg/src/transaction/upgrade_format_version.rs:
##########
@@ -0,0 +1,118 @@
+// 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::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};
+
+/// 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 {
+ format_version: Option<FormatVersion>,
+}
+
+impl UpgradeFormatVersionAction {
+ /// Creates a new `UpgradeFormatVersionAction` with no version set.
+ pub fn new() -> Self {
+ UpgradeFormatVersionAction {
+ format_version: None,
+ }
+ }
+
+ /// Sets the target format version for the upgrade.
+ ///
+ /// # Arguments
+ ///
+ /// * `format_version` - The version to upgrade the table format to.
+ ///
+ /// # Returns
+ ///
+ /// Returns the updated `UpgradeFormatVersionAction` with the format version set.
+ pub fn set_format_version(mut self, format_version: FormatVersion) -> Self {
+ self.format_version = Some(format_version);
+ self
+ }
+}
+
+impl Default for UpgradeFormatVersionAction {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+#[async_trait]
+impl TransactionAction for UpgradeFormatVersionAction {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ async fn commit(self: Arc<Self>, _table: &Table) -> Result<ActionCommit> {
+ if self.format_version.is_none() {
+ // error
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ "FormatVersion is not set for UpgradeFormatVersionAction!",
+ ));
+ }
+
+ Ok(ActionCommit::new(
+ vec![UpgradeFormatVersion {
+ format_version: self.format_version.unwrap(),
Review Comment:
Thanks for the suggestion, I think I was thinking in Java when writing thisπ
Fixed
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2139712392
##########
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:
```suggestion
for action in self.actions {
```
You don't need to clone whole collection, just clone each action, this saves a vec allocation.
##########
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 {
+ format_version: Option<FormatVersion>,
+}
+
+impl UpgradeFormatVersionAction {
+ /// Creates a new `UpgradeFormatVersionAction` with no version set.
+ pub fn new() -> Self {
+ UpgradeFormatVersionAction {
+ format_version: None,
+ }
+ }
+
+ /// Sets the target format version for the upgrade.
+ ///
+ /// # Arguments
+ ///
+ /// * `format_version` - The version to upgrade the table format to.
+ ///
+ /// # Returns
+ ///
+ /// Returns the updated `UpgradeFormatVersionAction` with the format version set.
+ pub fn set_format_version(mut self, format_version: FormatVersion) -> Self {
+ self.format_version = Some(format_version);
+ self
+ }
+}
+
+impl Default for UpgradeFormatVersionAction {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+#[async_trait]
+impl TransactionAction for UpgradeFormatVersionAction {
+ fn as_any(self: Arc<Self>) -> Arc<dyn Any> {
+ self
+ }
+
+ async fn commit(self: Arc<Self>, table: &Table) -> Result<ActionCommit> {
+ let current_version = table.metadata().format_version();
+ let updates: Vec<TableUpdate>;
+
+ if let Some(format_version) = self.format_version {
+ match current_version.cmp(&format_version) {
+ Ordering::Greater => {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Cannot downgrade table version from {} to {}",
+ current_version, format_version
+ ),
+ ));
+ }
+ Ordering::Less => {
+ updates = vec![UpgradeFormatVersion { format_version }];
+ }
+ Ordering::Equal => {
+ // do nothing
+ updates = vec![];
+ }
+ }
+ } else {
+ // error
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ "FormatVersion is not set for UpgradeFormatVersionAction!",
+ ));
+ }
Review Comment:
We don't need this, this check is already done here: https://github.com/apache/iceberg-rust/blob/3ea6f4791a2a4e15c4466cff7ccc478c08e2d3e3/crates/iceberg/src/spec/table_metadata_builder.rs#L211
##########
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:
We should not use assert here since this argument is user passed argument. We could do this check in `apply`
##########
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:
This maybe unnecessary, we could put it in `UpdateProperty`, see what's did in java: https://github.com/apache/iceberg/blob/afda8be25652d44d9339a79c6797b6bf20c55bd6/core/src/main/java/org/apache/iceberg/PropertiesUpdate.java#L73
##########
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:
```suggestion
fn as_any(&self) -> &Any {
self
}
```
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -104,32 +109,13 @@ impl Transaction {
}
/// Sets table to a new version.
- pub fn upgrade_table_version(mut self, format_version: FormatVersion) -> Result<Self> {
- let current_version = self.current_table.metadata().format_version();
- match current_version.cmp(&format_version) {
- Ordering::Greater => {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Cannot downgrade table version from {} to {}",
- current_version, format_version
- ),
- ));
- }
- Ordering::Less => {
- self.apply(vec![UpgradeFormatVersion { format_version }], vec![])?;
- }
- Ordering::Equal => {
- // Do nothing.
- }
- }
- Ok(self)
+ pub fn upgrade_table_version(&self) -> UpgradeFormatVersionAction {
+ UpgradeFormatVersionAction::new()
}
/// Update table's property.
- pub fn set_properties(mut self, props: HashMap<String, String>) -> Result<Self> {
- self.apply(vec![TableUpdate::SetProperties { updates: props }], vec![])?;
- Ok(self)
+ pub fn update_properties(&self) -> UpdatePropertiesAction {
Review Comment:
```suggestion
pub fn update_table_properties(&self) -> UpdatePropertiesAction {
```
##########
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));
+ self.updates.insert(key, value);
+ self
+ }
+
+ /// Adds a key to the removal set of this action.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the key was already marked for update.
+ ///
+ /// # Arguments
+ ///
+ /// * `key` - The property key to remove.
+ ///
+ /// # Returns
+ ///
+ /// The updated [`UpdatePropertiesAction`] with the key added to the removal set.
+ pub fn remove(mut self, key: String) -> Self {
+ assert!(!self.updates.contains_key(&key));
Review Comment:
Ditto.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2141223957
##########
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:
I'm not sure I understand the difference it makes by moving the condition check to `apply`. if the an user use it like below:
```
let action = tx.update_table_properties
action
.set(k1,v)
.remove(k1) // panic here
.apply(tx) // if the check was moved to apply, then we would panic here
```
I think the behavior is basically the same, and checking this condition early on can help users fail early since this error is non-retryable
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2143373042
##########
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:
Sure! Created an issue here to track: https://github.com/apache/iceberg-rust/issues/1437
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2144544211
##########
crates/iceberg/src/transaction/upgrade_format_version.rs:
##########
@@ -0,0 +1,118 @@
+// 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::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};
+
+/// 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 {
+ format_version: Option<FormatVersion>,
+}
+
+impl UpgradeFormatVersionAction {
+ /// Creates a new `UpgradeFormatVersionAction` with no version set.
+ pub fn new() -> Self {
+ UpgradeFormatVersionAction {
+ format_version: None,
+ }
+ }
+
+ /// Sets the target format version for the upgrade.
+ ///
+ /// # Arguments
+ ///
+ /// * `format_version` - The version to upgrade the table format to.
+ ///
+ /// # Returns
+ ///
+ /// Returns the updated `UpgradeFormatVersionAction` with the format version set.
+ pub fn set_format_version(mut self, format_version: FormatVersion) -> Self {
+ self.format_version = Some(format_version);
+ self
+ }
+}
+
+impl Default for UpgradeFormatVersionAction {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+#[async_trait]
+impl TransactionAction for UpgradeFormatVersionAction {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ async fn commit(self: Arc<Self>, _table: &Table) -> Result<ActionCommit> {
+ if self.format_version.is_none() {
+ // error
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ "FormatVersion is not set for UpgradeFormatVersionAction!",
+ ));
+ }
+
+ Ok(ActionCommit::new(
+ vec method to avoid using unwrap.
##########
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:
How about using [as_any](https://docs.rs/as-any/latest/as_any/)? This way we don't need to implement this for each action:
```
pub(crate) trait TransactionAction: AsAny {
...
}
```
##########
crates/iceberg/src/transaction/update_properties.rs:
##########
@@ -0,0 +1,149 @@
+// 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::{Error, ErrorKind, 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.
+ ///
+ /// # 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 {
+ self.updates.insert(key, value);
+ self
+ }
+
+ /// Adds a key to the removal set of this action.
+ ///
+ /// # Arguments
+ ///
+ /// * `key` - The property key to remove.
+ ///
+ /// # Returns
+ ///
+ /// The updated [`UpdatePropertiesAction`] with the key added to the removal set.
+ pub fn remove(mut self, key: String) -> Self {
+ self.removals.insert(key);
+ self
+ }
+}
+
+impl Default for UpdatePropertiesAction {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+#[async_trait]
+impl TransactionAction for UpdatePropertiesAction {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ async fn commit(self: Arc<Self>, _table: &Table) -> Result<ActionCommit> {
+ if let Some(overlapping_key) = self.removals.iter().find(|k| self.updates.contains_key(*k))
+ {
+ return Err(Error::new(
+ ErrorKind::PreconditionFailed,
+ format!(
+ "Key {} is present in both the HashSet and the HashMap",
Review Comment:
```suggestion
"Key {} is present in both removal set and update set",
```
##########
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:
Yeah, sound reasonable to me.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2141223957
##########
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:
I'm not sure I understand the difference it makes by moving the condition check to `apply`. if the an user use it like below:
```
let action = tx.update_table_properties
action
.set(k1,v)
.remove(k1) // panic here
.apply(tx) // if the check was moved to apply, then we would panic here
```
I think the effect is basically the same, and checking this condition early on can help users fail early
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2143366463
##########
crates/iceberg/src/transaction/action.rs:
##########
@@ -16,6 +16,8 @@
// under the License.
#![allow(dead_code)]
+
Review Comment:
Will remove, thanks for the reminder!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "dentiny (via GitHub)" <gi...@apache.org>.
dentiny commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2147428265
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -104,32 +110,13 @@ impl Transaction {
}
/// Sets table to a new version.
- pub fn upgrade_table_version(mut self, format_version: FormatVersion) -> Result<Self> {
- let current_version = self.current_table.metadata().format_version();
- match current_version.cmp(&format_version) {
- Ordering::Greater => {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Cannot downgrade table version from {} to {}",
- current_version, format_version
- ),
- ));
- }
- Ordering::Less => {
- self.apply(vec![UpgradeFormatVersion { format_version }], vec![])?;
- }
- Ordering::Equal => {
- // Do nothing.
- }
- }
- Ok(self)
+ pub fn upgrade_table_version(&self) -> UpgradeFormatVersionAction {
+ UpgradeFormatVersionAction::new()
}
/// Update table's property.
- pub fn set_properties(mut self, props: HashMap<String, String>) -> Result<Self> {
Review Comment:
Ooops, it's an interface change, one minor suggestion, better to call it out in PR description and later release notes. :)
It actually breaks [moonlink](https://github.com/Mooncake-Labs/moonlink) compilation :(
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2148859441
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -104,32 +110,13 @@ impl Transaction {
}
/// Sets table to a new version.
- pub fn upgrade_table_version(mut self, format_version: FormatVersion) -> Result<Self> {
- let current_version = self.current_table.metadata().format_version();
- match current_version.cmp(&format_version) {
- Ordering::Greater => {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Cannot downgrade table version from {} to {}",
- current_version, format_version
- ),
- ));
- }
- Ordering::Less => {
- self.apply(vec![UpgradeFormatVersion { format_version }], vec![])?;
- }
- Ordering::Equal => {
- // Do nothing.
- }
- }
- Ok(self)
+ pub fn upgrade_table_version(&self) -> UpgradeFormatVersionAction {
+ UpgradeFormatVersionAction::new()
}
/// Update table's property.
- pub fn set_properties(mut self, props: HashMap<String, String>) -> Result<Self> {
Review Comment:
Yes, we are making `TransactionAction`s retryable and had to change the existing APIs. There are more API changes coming for other actions like `ReplaceSortOrderAction`(https://github.com/apache/iceberg-rust/pull/1441) and `FastAppendAction`(WIP)
We definitely need to call this out in the next release notes. cc: @liurenjie1024
I'm not very familiar with moonlink and here goes the dumb question: Would it be better for moonlink to depend on a released/stable iceberg-rs version instead of tracking an unreleased git rev? You can set up a non-blocking github CI to track `iceberg-rs/main` if you want to catch compatibility issues early
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #1433:
URL: https://github.com/apache/iceberg-rust/pull/1433#discussion_r2149057838
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -104,32 +110,13 @@ impl Transaction {
}
/// Sets table to a new version.
- pub fn upgrade_table_version(mut self, format_version: FormatVersion) -> Result<Self> {
- let current_version = self.current_table.metadata().format_version();
- match current_version.cmp(&format_version) {
- Ordering::Greater => {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Cannot downgrade table version from {} to {}",
- current_version, format_version
- ),
- ));
- }
- Ordering::Less => {
- self.apply(vec![UpgradeFormatVersion { format_version }], vec![])?;
- }
- Ordering::Equal => {
- // Do nothing.
- }
- }
- Ok(self)
+ pub fn upgrade_table_version(&self) -> UpgradeFormatVersionAction {
+ UpgradeFormatVersionAction::new()
}
/// Update table's property.
- pub fn set_properties(mut self, props: HashMap<String, String>) -> Result<Self> {
Review Comment:
Thanks @dentiny for this comment. I thought about keeping the original api as deprecated, but the concern was to the overhead of maintaining it. But I do agree that we should shoutout this breaking change when release.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org