fix: correct errors in update properties implementation (#429)

Author: HeartLinked

src/iceberg/test/update_properties_test.cc

@@ -19,7 +19,6 @@
 
 #include "iceberg/update/update_properties.h"
 
-#include "iceberg/table.h"
 #include "iceberg/table_update.h"
 #include "iceberg/test/matchers.h"
 #include "iceberg/test/update_test_base.h"
@@ -28,11 +27,6 @@ namespace iceberg {
 
 class UpdatePropertiesTest : public UpdateTestBase {};
 
-TEST_F(UpdatePropertiesTest, EmptyUpdates) {
-  ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
-  EXPECT_THAT(update->Commit(), IsOk());
-}
-
 TEST_F(UpdatePropertiesTest, SetProperty) {
   ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
   update->Set("key1", "value1").Set("key2", "value2");
@@ -43,7 +37,14 @@ TEST_F(UpdatePropertiesTest, SetProperty) {
 }
 
 TEST_F(UpdatePropertiesTest, RemoveProperty) {
-  ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
+  // First, add properties to remove
+  ICEBERG_UNWRAP_OR_FAIL(auto setup_update, table_->NewUpdateProperties());
+  setup_update->Set("key1", "value1").Set("key2", "value2");
+  EXPECT_THAT(setup_update->Commit(), IsOk());
+
+  // Reload and remove the properties
+  ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_));
+  ICEBERG_UNWRAP_OR_FAIL(auto update, reloaded->NewUpdateProperties());
   update->Remove("key1").Remove("key2");
 
   ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply());
@@ -69,6 +70,17 @@ TEST_F(UpdatePropertiesTest, RemoveThenSetSameKey) {
   EXPECT_THAT(result, HasErrorMessage("already marked for removal"));
 }
 
+TEST_F(UpdatePropertiesTest, SetAndRemoveDifferentKeys) {
+  ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
+  update->Set("key1", "value1").Remove("key2");
+  EXPECT_THAT(update->Commit(), IsOk());
+
+  ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_));
+  const auto& props = reloaded->properties().configs();
+  EXPECT_EQ(props.at("key1"), "value1");
+  EXPECT_FALSE(props.contains("key2"));
+}
+
 TEST_F(UpdatePropertiesTest, UpgradeFormatVersionValid) {
   ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
   update->Set("format-version", "2");
@@ -108,33 +120,20 @@ TEST_F(UpdatePropertiesTest, UpgradeFormatVersionUnsupported) {
 }
 
 TEST_F(UpdatePropertiesTest, CommitSuccess) {
+  ICEBERG_UNWRAP_OR_FAIL(auto empty_update, table_->NewUpdateProperties());
+  EXPECT_THAT(empty_update->Commit(), IsOk());
+
   ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
   update->Set("new.property", "new.value");
+  update->Set("format-version", "3");
 
   EXPECT_THAT(update->Commit(), IsOk());
 
   ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_));
   const auto& props = reloaded->properties().configs();
   EXPECT_EQ(props.at("new.property"), "new.value");
-}
-
-TEST_F(UpdatePropertiesTest, FluentInterface) {
-  ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
-  auto& ref = update->Set("key1", "value1").Remove("key2");
-
-  EXPECT_EQ(&ref, update.get());
-  EXPECT_THAT(update->Apply(), IsOk());
-}
-
-TEST_F(UpdatePropertiesTest, SetAndRemoveDifferentKeys) {
-  ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
-  update->Set("key1", "value1").Remove("key2");
-  EXPECT_THAT(update->Commit(), IsOk());
-
-  ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_));
-  const auto& props = reloaded->properties().configs();
-  EXPECT_EQ(props.at("key1"), "value1");
-  EXPECT_FALSE(props.contains("key2"));
+  const auto& format_version = reloaded->metadata()->format_version;
+  EXPECT_EQ(format_version, 3);
 }
 
 }  // namespace iceberg

src/iceberg/update/update_properties.cc

@@ -56,7 +56,7 @@ UpdateProperties& UpdateProperties::Set(const std::string& key,
 
   if (!TableProperties::reserved_properties().contains(key) ||
       key == TableProperties::kFormatVersion.key()) {
-    updates_.emplace(key, value);
+    updates_.insert_or_assign(key, value);
   }
 
   return *this;
@@ -72,9 +72,21 @@ UpdateProperties& UpdateProperties::Remove(const std::string& key) {
 
 Result<PendingUpdate::ApplyResult> UpdateProperties::Apply() {
   ICEBERG_RETURN_UNEXPECTED(CheckErrors());
+  const auto& current_props = transaction_->current().properties.configs();
+  std::unordered_map<std::string, std::string> new_properties;
+  std::vector<std::string> removals;
+  for (const auto& [key, value] : current_props) {
+    if (!removals_.contains(key)) {
+      new_properties[key] = value;
+    }
+  }
+
+  for (const auto& [key, value] : updates_) {
+    new_properties[key] = value;
+  }
 
-  auto iter = updates_.find(TableProperties::kFormatVersion.key());
-  if (iter != updates_.end()) {
+  auto iter = new_properties.find(TableProperties::kFormatVersion.key());
+  if (iter != new_properties.end()) {
     int parsed_version = 0;
     const auto& val = iter->second;
     auto [ptr, ec] = std::from_chars(val.data(), val.data() + val.size(), parsed_version);
@@ -92,21 +104,27 @@ Result<PendingUpdate::ApplyResult> UpdateProperties::Apply() {
     }
     format_version_ = static_cast<int8_t>(parsed_version);
 
-    updates_.erase(iter);
+    updates_.erase(TableProperties::kFormatVersion.key());
   }
 
   if (auto schema = transaction_->current().Schema(); schema.has_value()) {
     ICEBERG_RETURN_UNEXPECTED(
-        MetricsConfig::VerifyReferencedColumns(updates_, *schema.value()));
+        MetricsConfig::VerifyReferencedColumns(new_properties, *schema.value()));
   }
 
   ApplyResult result;
   if (!updates_.empty()) {
     result.updates.emplace_back(std::make_unique<table::SetProperties>(updates_));
   }
   if (!removals_.empty()) {
-    result.updates.emplace_back(std::make_unique<table::RemoveProperties>(
-        std::vector<std::string>{removals_.begin(), removals_.end()}));
+    for (const auto& key : removals_) {
+      if (current_props.contains(key)) {
+        removals.push_back(key);
+      }
+    }
+    if (!removals.empty()) {
+      result.updates.emplace_back(std::make_unique<table::RemoveProperties>(removals));
+    }
   }
   if (format_version_.has_value()) {
     result.updates.emplace_back(

src/iceberg/update/update_properties.h

@@ -41,14 +41,16 @@ class ICEBERG_EXPORT UpdateProperties : public PendingUpdate {
 
   /// \brief Sets a property key to a specified value.
   ///
-  /// The key can not be marked for previous removal and reserved property keys will be
-  /// ignored.
+  /// The key must not have been previously marked for removal and reserved property keys
+  /// will be ignored.
+  ///
+  /// \param key The property key to set
+  /// \param value The property value to set
+  /// \return Reference to this UpdateProperties for chaining
   UpdateProperties& Set(const std::string& key, const std::string& value);
 
   /// \brief Marks a property for removal.
   ///
-  /// The key can not be already marked for removal.
-  ///
   /// \param key The property key to remove
   /// \return Reference to this UpdateProperties for chaining
   UpdateProperties& Remove(const std::string& key);