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-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
