Skip to content

Commit 64691e4

Browse files
zhjwpkuzhjwpku
authored
fix: reject tag refs for main snapshot ref (#745)
Validate that the reserved main snapshot ref is always a branch, and preserve snapshot log timestamps for newly added snapshots without overwriting table metadata update time. This behavior is aligned with Java's TableMetadata.Builder.setRef
1 parent f71f485 commit 64691e4

5 files changed

Lines changed: 98 additions & 11 deletions

File tree

‎src/iceberg/table_metadata.cc‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1078,7 +1078,6 @@ Status TableMetadataBuilder::Impl::AddSnapshot(std::shared_ptr<Snapshot> snapsho
10781078
"Cannot add snapshot with sequence number {} older than last sequence number {}",
10791079
snapshot->sequence_number, metadata_.last_sequence_number);
10801080

1081-
metadata_.last_updated_ms = snapshot->timestamp_ms;
10821081
metadata_.last_sequence_number = snapshot->sequence_number;
10831082
metadata_.snapshots.push_back(snapshot);
10841083
snapshots_by_id_.emplace(snapshot->snapshot_id, snapshot);
@@ -1167,22 +1166,26 @@ Status TableMetadataBuilder::Impl::SetRef(const std::string& name,
11671166
"Cannot set {} to unknown snapshot: {}", name, snapshot_id);
11681167
const auto& snapshot = snapshot_it->second;
11691168

1170-
// If snapshot was added in this set of changes, update last_updated_ms
1171-
if (std::ranges::any_of(changes_, [snapshot_id](const auto& change) {
1172-
return change->kind() == TableUpdate::Kind::kAddSnapshot &&
1173-
internal::checked_cast<const table::AddSnapshot&>(*change)
1174-
.snapshot()
1175-
->snapshot_id == snapshot_id;
1176-
})) {
1177-
metadata_.last_updated_ms = snapshot->timestamp_ms;
1178-
}
1169+
ICEBERG_CHECK(
1170+
name != SnapshotRef::kMainBranch || ref->type() == SnapshotRefType::kBranch,
1171+
"Cannot set {} to a tag, it must be a branch", SnapshotRef::kMainBranch);
11791172

11801173
if (name == SnapshotRef::kMainBranch) {
1174+
const bool is_added_snapshot =
1175+
std::ranges::any_of(changes_, [snapshot_id](const auto& change) {
1176+
return change->kind() == TableUpdate::Kind::kAddSnapshot &&
1177+
internal::checked_cast<const table::AddSnapshot&>(*change)
1178+
.snapshot()
1179+
->snapshot_id == snapshot_id;
1180+
});
11811181
metadata_.current_snapshot_id = ref->snapshot_id;
11821182
if (metadata_.last_updated_ms == kInvalidLastUpdatedMs) {
11831183
metadata_.last_updated_ms = CurrentTimePointMs();
11841184
}
1185-
metadata_.snapshot_log.emplace_back(metadata_.last_updated_ms, ref->snapshot_id);
1185+
1186+
auto time_of_change =
1187+
is_added_snapshot ? snapshot->timestamp_ms : metadata_.last_updated_ms;
1188+
metadata_.snapshot_log.emplace_back(time_of_change, ref->snapshot_id);
11861189
}
11871190

11881191
changes_.push_back(std::make_unique<table::SetSnapshotRef>(name, *ref));

‎src/iceberg/test/snapshot_manager_test.cc‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,25 @@ TEST_F(SnapshotManagerMinimalTableTest, CreateBranchOnEmptyTable) {
421421
EXPECT_EQ(it->second->type(), SnapshotRefType::kBranch);
422422
}
423423

424+
TEST_F(SnapshotManagerMinimalTableTest, CreateTagNamedMainFails) {
425+
ICEBERG_UNWRAP_OR_FAIL(auto manager, table_->NewSnapshotManager());
426+
manager->CreateBranch("branch1");
427+
ExpectCommitOk(manager->Commit());
428+
429+
auto metadata = ReloadMetadata();
430+
auto branch_it = metadata->refs.find("branch1");
431+
ASSERT_NE(branch_it, metadata->refs.end());
432+
ASSERT_EQ(branch_it->second->type(), SnapshotRefType::kBranch);
433+
434+
ICEBERG_UNWRAP_OR_FAIL(auto table_with_branch, catalog_->LoadTable(table_ident_));
435+
ICEBERG_UNWRAP_OR_FAIL(auto new_manager, table_with_branch->NewSnapshotManager());
436+
new_manager->CreateTag(std::string(SnapshotRef::kMainBranch),
437+
branch_it->second->snapshot_id);
438+
ExpectCommitError(new_manager->Commit(), ErrorKind::kValidationFailed,
439+
"Cannot set main to a tag, it must be a branch");
440+
ExpectNoRef(std::string(SnapshotRef::kMainBranch));
441+
}
442+
424443
TEST_F(SnapshotManagerMinimalTableTest,
425444
CreateBranchOnEmptyTableFailsWhenRefAlreadyExists) {
426445
ICEBERG_UNWRAP_OR_FAIL(auto manager, table_->NewSnapshotManager());

‎src/iceberg/test/table_metadata_builder_test.cc‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
#include "iceberg/test/matchers.h"
3737
#include "iceberg/transform.h"
3838
#include "iceberg/type.h"
39+
#include "iceberg/util/timepoint.h"
3940
#include "iceberg/util/uuid.h"
4041

4142
namespace iceberg {
@@ -1193,6 +1194,39 @@ TEST(TableMetadataBuilderTest, RemoveSnapshotRef) {
11931194
EXPECT_TRUE(metadata->refs.contains("ref1"));
11941195
}
11951196

1197+
TEST(TableMetadataBuilderTest, SetRefRejectsTagForMainBranch) {
1198+
auto base = CreateBaseMetadata();
1199+
auto builder = TableMetadataBuilder::BuildFrom(base.get());
1200+
1201+
builder->AddSnapshot(std::make_shared<Snapshot>(Snapshot{.snapshot_id = 1}));
1202+
ICEBERG_UNWRAP_OR_FAIL(auto main_tag, SnapshotRef::MakeTag(1));
1203+
1204+
builder->SetRef(std::string(SnapshotRef::kMainBranch), std::move(main_tag));
1205+
1206+
auto result = builder->Build();
1207+
ASSERT_THAT(result, IsError(ErrorKind::kValidationFailed));
1208+
EXPECT_THAT(result, HasErrorMessage("Cannot set main to a tag, it must be a branch"));
1209+
}
1210+
1211+
TEST(TableMetadataBuilderTest, SetMainRefToAddedSnapshotUsesSnapshotTimestampForLog) {
1212+
auto base = CreateBaseMetadata();
1213+
auto builder = TableMetadataBuilder::BuildFrom(base.get());
1214+
1215+
auto snapshot_time = TimePointMsFromUnixMs(123456789);
1216+
builder->AddSnapshot(std::make_shared<Snapshot>(
1217+
Snapshot{.snapshot_id = 1, .sequence_number = 1, .timestamp_ms = snapshot_time}));
1218+
ICEBERG_UNWRAP_OR_FAIL(auto main_branch, SnapshotRef::MakeBranch(1));
1219+
1220+
builder->SetRef(std::string(SnapshotRef::kMainBranch), std::move(main_branch));
1221+
1222+
ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build());
1223+
EXPECT_EQ(metadata->current_snapshot_id, 1);
1224+
EXPECT_NE(metadata->last_updated_ms, snapshot_time);
1225+
ASSERT_FALSE(metadata->snapshot_log.empty());
1226+
EXPECT_EQ(metadata->snapshot_log.back().snapshot_id, 1);
1227+
EXPECT_EQ(metadata->snapshot_log.back().timestamp_ms, snapshot_time);
1228+
}
1229+
11961230
TEST(TableMetadataBuilderTest, RemoveSnapshot) {
11971231
auto base = CreateBaseMetadata();
11981232
auto builder = TableMetadataBuilder::BuildFrom(base.get());

‎src/iceberg/test/table_update_test.cc‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -440,4 +440,24 @@ TEST(TableUpdateTest, SetSnapshotRefApplyUpdate) {
440440
}
441441
}
442442

443+
TEST(TableUpdateTest, SetSnapshotRefRejectsTagForMainBranch) {
444+
auto base = CreateBaseMetadata();
445+
auto builder = TableMetadataBuilder::BuildFrom(base.get());
446+
447+
auto snapshot = std::make_shared<Snapshot>(
448+
Snapshot{.snapshot_id = 987654321,
449+
.sequence_number = 1,
450+
.timestamp_ms = TimePointMsFromUnixMs(2000000),
451+
.manifest_list = "s3://bucket/manifest-list.avro"});
452+
builder->AddSnapshot(snapshot);
453+
454+
table::SetSnapshotRef update(std::string(SnapshotRef::kMainBranch), 987654321,
455+
SnapshotRefType::kTag);
456+
update.ApplyTo(*builder);
457+
458+
auto result = builder->Build();
459+
ASSERT_THAT(result, IsError(ErrorKind::kValidationFailed));
460+
EXPECT_THAT(result, HasErrorMessage("Cannot set main to a tag, it must be a branch"));
461+
}
462+
443463
} // namespace iceberg

‎src/iceberg/transaction.cc‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,13 +209,15 @@ Status Transaction::ApplyExpireSnapshots(ExpireSnapshots& update) {
209209
if (!result.schema_ids_to_remove.empty()) {
210210
ctx_->metadata_builder->RemoveSchemas(std::move(result.schema_ids_to_remove));
211211
}
212+
ICEBERG_RETURN_UNEXPECTED(ctx_->metadata_builder->CheckErrors());
212213
return {};
213214
}
214215

215216
Status Transaction::ApplySetSnapshot(SetSnapshot& update) {
216217
ICEBERG_ASSIGN_OR_RAISE(auto snapshot_id, update.Apply());
217218
ctx_->metadata_builder->SetBranchSnapshot(snapshot_id,
218219
std::string(SnapshotRef::kMainBranch));
220+
ICEBERG_RETURN_UNEXPECTED(ctx_->metadata_builder->CheckErrors());
219221
return {};
220222
}
221223

@@ -232,6 +234,7 @@ Status Transaction::ApplyUpdatePartitionSpec(UpdatePartitionSpec& update) {
232234
} else {
233235
ctx_->metadata_builder->AddPartitionSpec(std::move(result.spec));
234236
}
237+
ICEBERG_RETURN_UNEXPECTED(ctx_->metadata_builder->CheckErrors());
235238
return {};
236239
}
237240

@@ -246,6 +249,7 @@ Status Transaction::ApplyUpdateProperties(UpdateProperties& update) {
246249
if (result.format_version.has_value()) {
247250
ctx_->metadata_builder->UpgradeFormatVersion(result.format_version.value());
248251
}
252+
ICEBERG_RETURN_UNEXPECTED(ctx_->metadata_builder->CheckErrors());
249253
return {};
250254
}
251255

@@ -256,6 +260,7 @@ Status Transaction::ApplyUpdateSchema(UpdateSchema& update) {
256260
if (!result.updated_props.empty()) {
257261
ctx_->metadata_builder->SetProperties(result.updated_props);
258262
}
263+
ICEBERG_RETURN_UNEXPECTED(ctx_->metadata_builder->CheckErrors());
259264

260265
return {};
261266
}
@@ -275,6 +280,7 @@ Status Transaction::ApplyUpdateSnapshot(SnapshotUpdate& update) {
275280
} else {
276281
temp_update->SetBranchSnapshot(std::move(result.snapshot), result.target_branch);
277282
}
283+
ICEBERG_RETURN_UNEXPECTED(temp_update->CheckErrors());
278284

279285
if (temp_update->changes().empty()) {
280286
// Do not commit if the metadata has not changed. for example, this may happen
@@ -293,6 +299,7 @@ Status Transaction::ApplyUpdateSnapshot(SnapshotUpdate& update) {
293299
if (base.table_uuid.empty()) {
294300
ctx_->metadata_builder->AssignUUID();
295301
}
302+
ICEBERG_RETURN_UNEXPECTED(ctx_->metadata_builder->CheckErrors());
296303
return {};
297304
}
298305

@@ -304,12 +311,14 @@ Status Transaction::ApplyUpdateSnapshotReference(UpdateSnapshotReference& update
304311
for (auto&& [name, ref] : result.to_set) {
305312
ctx_->metadata_builder->SetRef(std::move(name), std::move(ref));
306313
}
314+
ICEBERG_RETURN_UNEXPECTED(ctx_->metadata_builder->CheckErrors());
307315
return {};
308316
}
309317

310318
Status Transaction::ApplyUpdateSortOrder(UpdateSortOrder& update) {
311319
ICEBERG_ASSIGN_OR_RAISE(auto sort_order, update.Apply());
312320
ctx_->metadata_builder->SetDefaultSortOrder(std::move(sort_order));
321+
ICEBERG_RETURN_UNEXPECTED(ctx_->metadata_builder->CheckErrors());
313322
return {};
314323
}
315324

@@ -321,6 +330,7 @@ Status Transaction::ApplyUpdateStatistics(UpdateStatistics& update) {
321330
for (const auto& snapshot_id : result.to_remove) {
322331
ctx_->metadata_builder->RemoveStatistics(snapshot_id);
323332
}
333+
ICEBERG_RETURN_UNEXPECTED(ctx_->metadata_builder->CheckErrors());
324334
return {};
325335
}
326336

@@ -332,6 +342,7 @@ Status Transaction::ApplyUpdatePartitionStatistics(UpdatePartitionStatistics& up
332342
for (const auto& snapshot_id : result.to_remove) {
333343
ctx_->metadata_builder->RemovePartitionStatistics(snapshot_id);
334344
}
345+
ICEBERG_RETURN_UNEXPECTED(ctx_->metadata_builder->CheckErrors());
335346
return {};
336347
}
337348

0 commit comments

Comments
 (0)