Skip to content

Commit 18a2516

Browse files
HeartLinkedHeartLinked
committed
fix: correct errors in update Properties implementation
1 parent 6f802aa commit 18a2516

File tree

3 files changed

+59
-37
lines changed

3 files changed

+59
-37
lines changed

‎src/iceberg/test/update_properties_test.cc‎

Lines changed: 27 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@
1919

2020
#include "iceberg/update/update_properties.h"
2121

22-
#include "iceberg/table.h"
22+
#include <gtest/gtest.h>
23+
2324
#include "iceberg/table_update.h"
2425
#include "iceberg/test/matchers.h"
2526
#include "iceberg/test/update_test_base.h"
@@ -28,11 +29,6 @@ namespace iceberg {
2829

2930
class UpdatePropertiesTest : public UpdateTestBase {};
3031

31-
TEST_F(UpdatePropertiesTest, EmptyUpdates) {
32-
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
33-
EXPECT_THAT(update->Commit(), IsOk());
34-
}
35-
3632
TEST_F(UpdatePropertiesTest, SetProperty) {
3733
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
3834
update->Set("key1", "value1").Set("key2", "value2");
@@ -43,7 +39,14 @@ TEST_F(UpdatePropertiesTest, SetProperty) {
4339
}
4440

4541
TEST_F(UpdatePropertiesTest, RemoveProperty) {
46-
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
42+
// First, add properties to remove
43+
ICEBERG_UNWRAP_OR_FAIL(auto setup_update, table_->NewUpdateProperties());
44+
setup_update->Set("key1", "value1").Set("key2", "value2");
45+
EXPECT_THAT(setup_update->Commit(), IsOk());
46+
47+
// Reload and remove the properties
48+
ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_));
49+
ICEBERG_UNWRAP_OR_FAIL(auto update, reloaded->NewUpdateProperties());
4750
update->Remove("key1").Remove("key2");
4851

4952
ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply());
@@ -69,6 +72,17 @@ TEST_F(UpdatePropertiesTest, RemoveThenSetSameKey) {
6972
EXPECT_THAT(result, HasErrorMessage("already marked for removal"));
7073
}
7174

75+
TEST_F(UpdatePropertiesTest, SetAndRemoveDifferentKeys) {
76+
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
77+
update->Set("key1", "value1").Remove("key2");
78+
EXPECT_THAT(update->Commit(), IsOk());
79+
80+
ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_));
81+
const auto& props = reloaded->properties().configs();
82+
EXPECT_EQ(props.at("key1"), "value1");
83+
EXPECT_FALSE(props.contains("key2"));
84+
}
85+
7286
TEST_F(UpdatePropertiesTest, UpgradeFormatVersionValid) {
7387
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
7488
update->Set("format-version", "2");
@@ -108,33 +122,20 @@ TEST_F(UpdatePropertiesTest, UpgradeFormatVersionUnsupported) {
108122
}
109123

110124
TEST_F(UpdatePropertiesTest, CommitSuccess) {
125+
ICEBERG_UNWRAP_OR_FAIL(auto empty_update, table_->NewUpdateProperties());
126+
EXPECT_THAT(empty_update->Commit(), IsOk());
127+
111128
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
112129
update->Set("new.property", "new.value");
130+
update->Set("format-version", "3");
113131

114132
EXPECT_THAT(update->Commit(), IsOk());
115133

116134
ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_));
117135
const auto& props = reloaded->properties().configs();
118136
EXPECT_EQ(props.at("new.property"), "new.value");
119-
}
120-
121-
TEST_F(UpdatePropertiesTest, FluentInterface) {
122-
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
123-
auto& ref = update->Set("key1", "value1").Remove("key2");
124-
125-
EXPECT_EQ(&ref, update.get());
126-
EXPECT_THAT(update->Apply(), IsOk());
127-
}
128-
129-
TEST_F(UpdatePropertiesTest, SetAndRemoveDifferentKeys) {
130-
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties());
131-
update->Set("key1", "value1").Remove("key2");
132-
EXPECT_THAT(update->Commit(), IsOk());
133-
134-
ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_));
135-
const auto& props = reloaded->properties().configs();
136-
EXPECT_EQ(props.at("key1"), "value1");
137-
EXPECT_FALSE(props.contains("key2"));
137+
const auto& format_version = reloaded->metadata()->format_version;
138+
EXPECT_EQ(format_version, 3);
138139
}
139140

140141
} // namespace iceberg

‎src/iceberg/update/update_properties.cc‎

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ UpdateProperties& UpdateProperties::Set(const std::string& key,
5656

5757
if (!TableProperties::reserved_properties().contains(key) ||
5858
key == TableProperties::kFormatVersion.key()) {
59-
updates_.emplace(key, value);
59+
updates_.insert_or_assign(key, value);
6060
}
6161

6262
return *this;
@@ -72,9 +72,20 @@ UpdateProperties& UpdateProperties::Remove(const std::string& key) {
7272

7373
Result<PendingUpdate::ApplyResult> UpdateProperties::Apply() {
7474
ICEBERG_RETURN_UNEXPECTED(CheckErrors());
75+
const auto& current_props = transaction_->current().properties.configs();
76+
std::unordered_map<std::string, std::string> new_properties;
77+
for (const auto& [key, value] : current_props) {
78+
if (!removals_.contains(key)) {
79+
new_properties[key] = value;
80+
}
81+
}
82+
83+
for (const auto& [key, value] : updates_) {
84+
new_properties[key] = value;
85+
}
7586

76-
auto iter = updates_.find(TableProperties::kFormatVersion.key());
77-
if (iter != updates_.end()) {
87+
auto iter = new_properties.find(TableProperties::kFormatVersion.key());
88+
if (iter != new_properties.end()) {
7889
int parsed_version = 0;
7990
const auto& val = iter->second;
8091
auto [ptr, ec] = std::from_chars(val.data(), val.data() + val.size(), parsed_version);
@@ -92,21 +103,29 @@ Result<PendingUpdate::ApplyResult> UpdateProperties::Apply() {
92103
}
93104
format_version_ = static_cast<int8_t>(parsed_version);
94105

95-
updates_.erase(iter);
106+
updates_.erase(TableProperties::kFormatVersion.key());
96107
}
97108

98109
if (auto schema = transaction_->current().Schema(); schema.has_value()) {
99110
ICEBERG_RETURN_UNEXPECTED(
100-
MetricsConfig::VerifyReferencedColumns(updates_, *schema.value()));
111+
MetricsConfig::VerifyReferencedColumns(new_properties, *schema.value()));
101112
}
102113

103114
ApplyResult result;
104115
if (!updates_.empty()) {
105116
result.updates.emplace_back(std::make_unique<table::SetProperties>(updates_));
106117
}
107118
if (!removals_.empty()) {
108-
result.updates.emplace_back(std::make_unique<table::RemoveProperties>(
109-
std::vector<std::string>{removals_.begin(), removals_.end()}));
119+
std::vector<std::string> existing_removals;
120+
for (const auto& key : removals_) {
121+
if (current_props.contains(key)) {
122+
existing_removals.push_back(key);
123+
}
124+
}
125+
if (!existing_removals.empty()) {
126+
result.updates.emplace_back(
127+
std::make_unique<table::RemoveProperties>(existing_removals));
128+
}
110129
}
111130
if (format_version_.has_value()) {
112131
result.updates.emplace_back(

‎src/iceberg/update/update_properties.h‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,16 @@ class ICEBERG_EXPORT UpdateProperties : public PendingUpdate {
4141

4242
/// \brief Sets a property key to a specified value.
4343
///
44-
/// The key can not be marked for previous removal and reserved property keys will be
45-
/// ignored.
44+
/// The key must not have been previously marked for removal and reserved property keys
45+
/// will be ignored.
46+
///
47+
/// \param key The property key to set
48+
/// \param value The property value to set
49+
/// \return Reference to this UpdateProperties for chaining
4650
UpdateProperties& Set(const std::string& key, const std::string& value);
4751

4852
/// \brief Marks a property for removal.
4953
///
50-
/// The key can not be already marked for removal.
51-
///
5254
/// \param key The property key to remove
5355
/// \return Reference to this UpdateProperties for chaining
5456
UpdateProperties& Remove(const std::string& key);

0 commit comments

Comments
 (0)