fix: Correct errors in update Properties implementation

Author: HeartLinked

src/iceberg/test/update_properties_test.cc

@@ -26,6 +26,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
+#include "gmock/gmock.h"
 #include "iceberg/file_format.h"
 #include "iceberg/result.h"
 #include "iceberg/schema.h"
@@ -60,19 +61,18 @@ class UpdatePropertiesTest : public ::testing::Test {
   TableIdentifier identifier_;
 };
 
-TEST_F(UpdatePropertiesTest, EmptyUpdates) {
-  UpdateProperties update(identifier_, catalog_, metadata_);
-
-  auto result = update.Commit();
-  EXPECT_THAT(result, IsOk());
-}
-
 TEST_F(UpdatePropertiesTest, SetProperty) {
   UpdateProperties update(identifier_, catalog_, metadata_);
   update.Set("key1", "value1").Set("key2", "value2");
 
   auto result = update.Apply();
   EXPECT_THAT(result, IsOk());
+
+  // Verify the actual pending updates
+  const auto& pending_updates = update.GetPendingUpdates();
+  EXPECT_EQ(pending_updates.size(), 2);
+  EXPECT_EQ(pending_updates.at("key1"), "value1");
+  EXPECT_EQ(pending_updates.at("key2"), "value2");
 }
 
 TEST_F(UpdatePropertiesTest, RemoveProperty) {
@@ -81,6 +81,12 @@ TEST_F(UpdatePropertiesTest, RemoveProperty) {
 
   auto result = update.Apply();
   EXPECT_THAT(result, IsOk());
+
+  // Verify the actual pending removals
+  const auto& pending_removals = update.GetPendingRemovals();
+  EXPECT_EQ(pending_removals.size(), 2);
+  EXPECT_TRUE(pending_removals.contains("key1"));
+  EXPECT_TRUE(pending_removals.contains("key2"));
 }
 
 TEST_F(UpdatePropertiesTest, SetRemoveConflict) {
@@ -113,6 +119,15 @@ TEST_F(UpdatePropertiesTest, UpgradeFormatVersion) {
 
     auto result = update.Apply();
     EXPECT_THAT(result, IsOk());
+
+    // Verify the format version is set correctly
+    const auto& format_version = update.GetPendingFormatVersion();
+    ASSERT_TRUE(format_version.has_value());
+    EXPECT_EQ(format_version.value(), 2);
+
+    // format-version should be removed from pending updates after Apply()
+    const auto& pending_updates = update.GetPendingUpdates();
+    EXPECT_FALSE(pending_updates.contains("format-version"));
   }
 
   {
@@ -151,10 +166,15 @@ TEST_F(UpdatePropertiesTest, InvalidTable) {
   {
     // catalog is null
     UpdateProperties update(identifier_, nullptr, metadata_);
-
-    auto result = update.Apply();
-    EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument));
-    EXPECT_THAT(result, HasErrorMessage("Catalog is required"));
+    update.Set("key1", "value1");
+    auto apply_result = update.Apply();
+    EXPECT_THAT(apply_result, IsOk());
+
+    // Commit should fail since catalog is required
+    auto commit_result = update.Commit();
+    EXPECT_THAT(commit_result, IsError(ErrorKind::kInvalidArgument));
+    EXPECT_THAT(commit_result,
+                HasErrorMessage("Catalog is required to commit property updates"));
   }
 
   {
@@ -201,6 +221,15 @@ TEST_F(UpdatePropertiesTest, FluentInterface) {
 
   auto result = update.Apply();
   EXPECT_THAT(result, IsOk());
+
+  // Verify the actual pending updates and removals
+  const auto& pending_updates = update.GetPendingUpdates();
+  EXPECT_EQ(pending_updates.size(), 1);
+  EXPECT_EQ(pending_updates.at("key1"), "value1");
+
+  const auto& pending_removals = update.GetPendingRemovals();
+  EXPECT_EQ(pending_removals.size(), 1);
+  EXPECT_TRUE(pending_removals.contains("key2"));
 }
 
 }  // namespace iceberg

src/iceberg/update/update_properties.cc

@@ -71,17 +71,25 @@ UpdateProperties& UpdateProperties::Remove(const std::string& key) {
 }
 
 Status UpdateProperties::Apply() {
-  if (!catalog_) {
-    return InvalidArgument("Catalog is required to apply property updates");
-  }
   if (!base_metadata_) {
     return InvalidArgument("Base table metadata is required to apply property updates");
   }
 
   ICEBERG_RETURN_UNEXPECTED(CheckErrors());
 
-  auto iter = updates_.find(TableProperties::kFormatVersion.key());
-  if (iter != updates_.end()) {
+  std::unordered_map<std::string, std::string> new_properties;
+  for (const auto& [key, value] : base_metadata_->properties.configs()) {
+    if (!removals_.contains(key)) {
+      new_properties[key] = value;
+    }
+  }
+
+  for (const auto& [key, value] : updates_) {
+    new_properties[key] = value;
+  }
+
+  auto iter = new_properties.find(TableProperties::kFormatVersion.key());
+  if (iter != new_properties.end()) {
     try {
       int parsed_version = std::stoi(iter->second);
       if (parsed_version > TableMetadata::kSupportedTableFormatVersion) {
@@ -97,19 +105,23 @@ Status UpdateProperties::Apply() {
       return InvalidArgument("Format version '{}' is out of range", iter->second);
     }
 
-    updates_.erase(iter);
+    updates_.erase(TableProperties::kFormatVersion.key());
   }
 
   if (auto schema = base_metadata_->Schema(); schema.has_value()) {
     ICEBERG_RETURN_UNEXPECTED(
-        MetricsConfig::VerifyReferencedColumns(updates_, *schema.value()));
+        MetricsConfig::VerifyReferencedColumns(new_properties, *schema.value()));
   }
   return {};
 }
 
 Status UpdateProperties::Commit() {
   ICEBERG_RETURN_UNEXPECTED(Apply());
 
+  if (!catalog_) {
+    return InvalidArgument("Catalog is required to commit property updates");
+  }
+
   std::vector<std::unique_ptr<TableUpdate>> updates;
   if (!updates_.empty()) {
     updates.emplace_back(std::make_unique<table::SetProperties>(std::move(updates_)));
@@ -131,4 +143,17 @@ Status UpdateProperties::Commit() {
   return {};
 }
 
+const std::unordered_map<std::string, std::string>& UpdateProperties::GetPendingUpdates()
+    const {
+  return updates_;
+}
+
+const std::unordered_set<std::string>& UpdateProperties::GetPendingRemovals() const {
+  return removals_;
+}
+
+const std::optional<int8_t>& UpdateProperties::GetPendingFormatVersion() const {
+  return format_version_;
+}
+
 }  // namespace iceberg

src/iceberg/update/update_properties.h

@@ -24,7 +24,6 @@
 #include <unordered_map>
 #include <unordered_set>
 
-#include "iceberg/file_format.h"
 #include "iceberg/iceberg_export.h"
 #include "iceberg/pending_update.h"
 #include "iceberg/table_identifier.h"
@@ -45,14 +44,15 @@ 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);
@@ -69,9 +69,24 @@ class ICEBERG_EXPORT UpdateProperties : public PendingUpdate {
   ///
   /// Validates the changes and applies them to the table through the catalog.
   ///
-  /// \return OK if the changes are valid and committed successfully, or an error
+  /// \return Status::OK if the changes are valid and committed successfully, or an error.
   Status Commit() override;
 
+  /// \brief Get the pending property updates after applying changes.
+  ///
+  /// \return The map of property updates
+  const std::unordered_map<std::string, std::string>& GetPendingUpdates() const;
+
+  /// \brief Get the pending property removals after applying changes.
+  ///
+  /// \return The set of property keys to remove
+  const std::unordered_set<std::string>& GetPendingRemovals() const;
+
+  /// \brief Get the pending format version upgrade after applying changes.
+  ///
+  /// \return The optional format version
+  const std::optional<int8_t>& GetPendingFormatVersion() const;
+
  private:
   TableIdentifier identifier_;
   std::shared_ptr<Catalog> catalog_;