diff --git a/src/iceberg/test/update_properties_test.cc b/src/iceberg/test/update_properties_test.cc index 1084f08fd..5350af422 100644 --- a/src/iceberg/test/update_properties_test.cc +++ b/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 diff --git a/src/iceberg/update/update_properties.cc b/src/iceberg/update/update_properties.cc index f8a1d85d5..9cdd7b5a7 100644 --- a/src/iceberg/update/update_properties.cc +++ b/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 UpdateProperties::Apply() { ICEBERG_RETURN_UNEXPECTED(CheckErrors()); + const auto& current_props = transaction_->current().properties.configs(); + std::unordered_map new_properties; + std::vector 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,12 +104,12 @@ Result UpdateProperties::Apply() { } format_version_ = static_cast(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; @@ -105,8 +117,14 @@ Result UpdateProperties::Apply() { result.updates.emplace_back(std::make_unique(updates_)); } if (!removals_.empty()) { - result.updates.emplace_back(std::make_unique( - std::vector{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(removals)); + } } if (format_version_.has_value()) { result.updates.emplace_back( diff --git a/src/iceberg/update/update_properties.h b/src/iceberg/update/update_properties.h index fc8f46f1a..e0f578bea 100644 --- a/src/iceberg/update/update_properties.h +++ b/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);