diff --git a/src/iceberg/test/update_properties_test.cc b/src/iceberg/test/update_properties_test.cc index 13cfec831..befae5360 100644 --- a/src/iceberg/test/update_properties_test.cc +++ b/src/iceberg/test/update_properties_test.cc @@ -26,6 +26,7 @@ #include #include +#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 diff --git a/src/iceberg/update/update_properties.cc b/src/iceberg/update/update_properties.cc index a4dcd1548..8169d929d 100644 --- a/src/iceberg/update/update_properties.cc +++ b/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 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,12 +105,12 @@ 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 {}; } @@ -110,6 +118,10 @@ Status UpdateProperties::Apply() { Status UpdateProperties::Commit() { ICEBERG_RETURN_UNEXPECTED(Apply()); + if (!catalog_) { + return InvalidArgument("Catalog is required to commit property updates"); + } + std::vector> updates; if (!updates_.empty()) { updates.emplace_back(std::make_unique(std::move(updates_))); @@ -131,4 +143,17 @@ Status UpdateProperties::Commit() { return {}; } +const std::unordered_map& UpdateProperties::GetPendingUpdates() + const { + return updates_; +} + +const std::unordered_set& UpdateProperties::GetPendingRemovals() const { + return removals_; +} + +const std::optional& UpdateProperties::GetPendingFormatVersion() const { + return format_version_; +} + } // namespace iceberg diff --git a/src/iceberg/update/update_properties.h b/src/iceberg/update/update_properties.h index 0f1adf76a..de4f1e663 100644 --- a/src/iceberg/update/update_properties.h +++ b/src/iceberg/update/update_properties.h @@ -20,11 +20,11 @@ #pragma once #include +#include #include #include #include -#include "iceberg/file_format.h" #include "iceberg/iceberg_export.h" #include "iceberg/pending_update.h" #include "iceberg/table_identifier.h" @@ -45,14 +45,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 +70,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& GetPendingUpdates() const; + + /// \brief Get the pending property removals after applying changes. + /// + /// \return The set of property keys to remove + const std::unordered_set& GetPendingRemovals() const; + + /// \brief Get the pending format version upgrade after applying changes. + /// + /// \return The optional format version + const std::optional& GetPendingFormatVersion() const; + private: TableIdentifier identifier_; std::shared_ptr catalog_;