This is an automated email from the ASF dual-hosted git repository.
gangwu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new 9c00bfae fix: correct errors in update properties implementation (#429)
9c00bfae is described below
commit 9c00bfaef9167d47748265497a92093cccef52b5
Author: Feiyang Li <[email protected]>
AuthorDate: Tue Dec 23 11:14:53 2025 +0800
fix: correct errors in update properties implementation (#429)
---
src/iceberg/test/update_properties_test.cc | 51 +++++++++++++++---------------
src/iceberg/update/update_properties.cc | 32 +++++++++++++++----
src/iceberg/update/update_properties.h | 10 +++---
3 files changed, 56 insertions(+), 37 deletions(-)
diff --git a/src/iceberg/test/update_properties_test.cc
b/src/iceberg/test/update_properties_test.cc
index 1084f08f..5350af42 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 f8a1d85d..9cdd7b5a 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<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,12 +104,12 @@ 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;
@@ -105,8 +117,14 @@ Result<PendingUpdate::ApplyResult>
UpdateProperties::Apply() {
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(
diff --git a/src/iceberg/update/update_properties.h
b/src/iceberg/update/update_properties.h
index fc8f46f1..e0f578be 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);
