From 18a25164ff5dfa2d3b18dde159e574c428e89a82 Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Mon, 22 Dec 2025 16:02:47 +0800 Subject: [PATCH 1/3] fix: correct errors in update Properties implementation --- src/iceberg/test/update_properties_test.cc | 53 +++++++++++----------- src/iceberg/update/update_properties.cc | 33 +++++++++++--- src/iceberg/update/update_properties.h | 10 ++-- 3 files changed, 59 insertions(+), 37 deletions(-) diff --git a/src/iceberg/test/update_properties_test.cc b/src/iceberg/test/update_properties_test.cc index 1084f08fd..54fd44925 100644 --- a/src/iceberg/test/update_properties_test.cc +++ b/src/iceberg/test/update_properties_test.cc @@ -19,7 +19,8 @@ #include "iceberg/update/update_properties.h" -#include "iceberg/table.h" +#include + #include "iceberg/table_update.h" #include "iceberg/test/matchers.h" #include "iceberg/test/update_test_base.h" @@ -28,11 +29,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 +39,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 +72,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 +122,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..e29336691 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,20 @@ 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; + 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 +103,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 +116,16 @@ 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()})); + std::vector existing_removals; + for (const auto& key : removals_) { + if (current_props.contains(key)) { + existing_removals.push_back(key); + } + } + if (!existing_removals.empty()) { + result.updates.emplace_back( + std::make_unique(existing_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); From 8c076c06608eb8f7a2539250196f494078d4c735 Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Mon, 22 Dec 2025 16:05:21 +0800 Subject: [PATCH 2/3] 1 --- src/iceberg/test/update_properties_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/iceberg/test/update_properties_test.cc b/src/iceberg/test/update_properties_test.cc index 54fd44925..5350af422 100644 --- a/src/iceberg/test/update_properties_test.cc +++ b/src/iceberg/test/update_properties_test.cc @@ -19,8 +19,6 @@ #include "iceberg/update/update_properties.h" -#include - #include "iceberg/table_update.h" #include "iceberg/test/matchers.h" #include "iceberg/test/update_test_base.h" From 0b6286a275dfc2a6aac4d7c903d4a729df3a68dc Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Mon, 22 Dec 2025 17:06:28 +0800 Subject: [PATCH 3/3] 1 --- src/iceberg/update/update_properties.cc | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/iceberg/update/update_properties.cc b/src/iceberg/update/update_properties.cc index e29336691..9cdd7b5a7 100644 --- a/src/iceberg/update/update_properties.cc +++ b/src/iceberg/update/update_properties.cc @@ -74,6 +74,7 @@ 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; @@ -116,15 +117,13 @@ Result UpdateProperties::Apply() { result.updates.emplace_back(std::make_unique(updates_)); } if (!removals_.empty()) { - std::vector existing_removals; for (const auto& key : removals_) { if (current_props.contains(key)) { - existing_removals.push_back(key); + removals.push_back(key); } } - if (!existing_removals.empty()) { - result.updates.emplace_back( - std::make_unique(existing_removals)); + if (!removals.empty()) { + result.updates.emplace_back(std::make_unique(removals)); } } if (format_version_.has_value()) {