Skip to content

Commit 8388f32

Browse files
shangxinlishangxinli
authored
feat: implement validation for table update requirements (#294)
Implement Validate() methods for remaining table requirement classes: - AssertDoesNotExist: validates table doesn't exist - AssertRefSnapshotID: validates snapshot references (branches/tags) - AssertLastAssignedFieldId: validates last assigned field ID - AssertLastAssignedPartitionId: validates last assigned partition ID - AssertDefaultSpecID: validates default partition spec ID - AssertDefaultSortOrderID: validates default sort order ID All implementations follow the pattern established in AssertCurrentSchemaID and provide descriptive error messages for validation failures.
1 parent 92cbbda commit 8388f32

File tree

2 files changed

+224
-15
lines changed

2 files changed

+224
-15
lines changed

‎src/iceberg/table_requirement.cc‎

Lines changed: 75 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -19,43 +19,73 @@
1919

2020
#include "iceberg/table_requirement.h"
2121

22+
#include "iceberg/snapshot.h"
2223
#include "iceberg/table_metadata.h"
24+
#include "iceberg/util/macros.h"
2325
#include "iceberg/util/string_util.h"
2426

2527
namespace iceberg::table {
2628

2729
Status AssertDoesNotExist::Validate(const TableMetadata* base) const {
28-
return NotImplemented("AssertDoesNotExist::Validate not implemented");
30+
if (base != nullptr) {
31+
return CommitFailed("Requirement failed: table already exists");
32+
}
33+
34+
return {};
2935
}
3036

3137
Status AssertUUID::Validate(const TableMetadata* base) const {
32-
// Validate that the table UUID matches the expected value
33-
3438
if (base == nullptr) {
3539
return CommitFailed("Requirement failed: current table metadata is missing");
3640
}
3741

3842
if (!StringUtils::EqualsIgnoreCase(base->table_uuid, uuid_)) {
3943
return CommitFailed(
40-
"Requirement failed: table UUID does not match (expected='{}', actual='{}')",
41-
uuid_, base->table_uuid);
44+
"Requirement failed: table UUID does not match, expected {} != {}", uuid_,
45+
base->table_uuid);
4246
}
4347

4448
return {};
4549
}
4650

4751
Status AssertRefSnapshotID::Validate(const TableMetadata* base) const {
48-
return NotImplemented("AssertTableRefSnapshotID::Validate not implemented");
52+
if (base == nullptr) {
53+
return CommitFailed("Requirement failed: current table metadata is missing");
54+
}
55+
56+
if (auto ref = base->refs.find(ref_name_); ref != base->refs.cend()) {
57+
const auto& snapshot_ref = ref->second;
58+
ICEBERG_DCHECK(snapshot_ref != nullptr, "Snapshot reference is null");
59+
std::string_view type =
60+
snapshot_ref->type() == SnapshotRefType::kBranch ? "branch" : "tag";
61+
if (!snapshot_id_.has_value()) {
62+
// A null snapshot ID means the ref should not exist already
63+
return CommitFailed("Requirement failed: {} '{}' was created concurrently", type,
64+
ref_name_);
65+
} else if (snapshot_id_.value() != snapshot_ref->snapshot_id) {
66+
return CommitFailed("Requirement failed: {} '{}' has changed: expected id {} != {}",
67+
type, ref_name_, snapshot_id_.value(),
68+
snapshot_ref->snapshot_id);
69+
}
70+
} else if (snapshot_id_.has_value()) {
71+
return CommitFailed("Requirement failed: branch or tag '{}' is missing, expected {}",
72+
ref_name_, snapshot_id_.value());
73+
}
74+
75+
return {};
4976
}
5077

5178
Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const {
52-
return NotImplemented(
53-
"AssertCurrentTableLastAssignedFieldId::Validate not implemented");
79+
if (base && base->last_column_id != last_assigned_field_id_) {
80+
return CommitFailed(
81+
"Requirement failed: last assigned field ID does not match, expected {} != {}",
82+
last_assigned_field_id_, base->last_column_id);
83+
}
84+
85+
return {};
5486
}
5587

5688
Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const {
57-
// Validate that the current schema ID matches the one used when the metadata was read
58-
5989
if (base == nullptr) {
6090
return CommitFailed("Requirement failed: current table metadata is missing");
6191
}
@@ -67,24 +97,54 @@ Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const {
6797

6898
if (base->current_schema_id.value() != schema_id_) {
6999
return CommitFailed(
70-
"Requirement failed: current schema ID does not match (expected={}, actual={})",
100+
"Requirement failed: current schema ID does not match, expected {} != {}",
71101
schema_id_, base->current_schema_id.value());
72102
}
73103

74104
return {};
75105
}
76106

77107
Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const {
78-
return NotImplemented(
79-
"AssertCurrentTableLastAssignedPartitionId::Validate not implemented");
108+
if (base == nullptr) {
109+
return CommitFailed("Requirement failed: current table metadata is missing");
110+
}
111+
112+
if (base && base->last_partition_id != last_assigned_partition_id_) {
113+
return CommitFailed(
114+
"Requirement failed: last assigned partition ID does not match, expected {} != "
115+
"{}",
116+
last_assigned_partition_id_, base->last_partition_id);
117+
}
118+
119+
return {};
80120
}
81121

82122
Status AssertDefaultSpecID::Validate(const TableMetadata* base) const {
83-
return NotImplemented("AssertDefaultTableSpecID::Validate not implemented");
123+
if (base == nullptr) {
124+
return CommitFailed("Requirement failed: current table metadata is missing");
125+
}
126+
127+
if (base->default_spec_id != spec_id_) {
128+
return CommitFailed(
129+
"Requirement failed: default partition spec changed, expected id {} != {}",
130+
spec_id_, base->default_spec_id);
131+
}
132+
133+
return {};
84134
}
85135

86136
Status AssertDefaultSortOrderID::Validate(const TableMetadata* base) const {
87-
return NotImplemented("AssertDefaultTableSortOrderID::Validate not implemented");
137+
if (base == nullptr) {
138+
return CommitFailed("Requirement failed: current table metadata is missing");
139+
}
140+
141+
if (base->default_sort_order_id != sort_order_id_) {
142+
return CommitFailed(
143+
"Requirement failed: default sort order changed: expected id {} != {}",
144+
sort_order_id_, base->default_sort_order_id);
145+
}
146+
147+
return {};
88148
}
89149

90150
} // namespace iceberg::table

‎src/iceberg/test/table_metadata_builder_test.cc‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,155 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertCurrentSchemaIDNotSet) {
264264
EXPECT_THAT(status, HasErrorMessage("schema ID is not set"));
265265
}
266266

267+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDoesNotExistSuccess) {
268+
table::AssertDoesNotExist requirement;
269+
270+
ASSERT_THAT(requirement.Validate(nullptr), IsOk());
271+
}
272+
273+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDoesNotExistTableExists) {
274+
table::AssertDoesNotExist requirement;
275+
276+
auto status = requirement.Validate(base_metadata_.get());
277+
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
278+
EXPECT_THAT(status, HasErrorMessage("table already exists"));
279+
}
280+
281+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDSuccess) {
282+
auto ref = std::make_shared<SnapshotRef>();
283+
ref->snapshot_id = 100;
284+
ref->retention = SnapshotRef::Branch{};
285+
base_metadata_->refs["main"] = ref;
286+
287+
table::AssertRefSnapshotID requirement("main", 100);
288+
289+
ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk());
290+
}
291+
292+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDMismatch) {
293+
auto ref = std::make_shared<SnapshotRef>();
294+
ref->snapshot_id = 100;
295+
ref->retention = SnapshotRef::Branch{};
296+
base_metadata_->refs["main"] = ref;
297+
298+
table::AssertRefSnapshotID requirement("main", 200);
299+
300+
auto status = requirement.Validate(base_metadata_.get());
301+
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
302+
EXPECT_THAT(status, HasErrorMessage("has changed"));
303+
}
304+
305+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDRefMissing) {
306+
table::AssertRefSnapshotID requirement("missing-ref", 100);
307+
308+
auto status = requirement.Validate(base_metadata_.get());
309+
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
310+
EXPECT_THAT(status, HasErrorMessage("is missing"));
311+
}
312+
313+
// Removed TableRequirementAssertRefSnapshotIDNullBase test
314+
// Java implementation doesn't check for null base, so passing nullptr would cause
315+
// undefined behavior This matches Java's assumption that base is never null when Validate
316+
// is called
317+
318+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptSuccess) {
319+
// Ref should not exist, and it doesn't
320+
table::AssertRefSnapshotID requirement("nonexistent", std::nullopt);
321+
322+
ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk());
323+
}
324+
325+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptButExists) {
326+
auto ref = std::make_shared<SnapshotRef>();
327+
ref->snapshot_id = 100;
328+
ref->retention = SnapshotRef::Branch{};
329+
base_metadata_->refs["main"] = ref;
330+
331+
table::AssertRefSnapshotID requirement("main", std::nullopt);
332+
333+
auto status = requirement.Validate(base_metadata_.get());
334+
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
335+
EXPECT_THAT(status, HasErrorMessage("created concurrently"));
336+
}
337+
338+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdSuccess) {
339+
base_metadata_->last_column_id = 10;
340+
table::AssertLastAssignedFieldId requirement(10);
341+
342+
ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk());
343+
}
344+
345+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdMismatch) {
346+
base_metadata_->last_column_id = 10;
347+
table::AssertLastAssignedFieldId requirement(15);
348+
349+
auto status = requirement.Validate(base_metadata_.get());
350+
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
351+
EXPECT_THAT(status, HasErrorMessage("last assigned field ID does not match"));
352+
}
353+
354+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdNullBase) {
355+
table::AssertLastAssignedFieldId requirement(10);
356+
357+
EXPECT_THAT(requirement.Validate(nullptr), IsOk());
358+
}
359+
360+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdSuccess) {
361+
base_metadata_->last_partition_id = 5;
362+
table::AssertLastAssignedPartitionId requirement(5);
363+
364+
ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk());
365+
}
366+
367+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdMismatch) {
368+
base_metadata_->last_partition_id = 5;
369+
table::AssertLastAssignedPartitionId requirement(8);
370+
371+
auto status = requirement.Validate(base_metadata_.get());
372+
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
373+
EXPECT_THAT(status, HasErrorMessage("last assigned partition ID does not match"));
374+
}
375+
376+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdNullBase) {
377+
table::AssertLastAssignedPartitionId requirement(5);
378+
379+
auto status = requirement.Validate(nullptr);
380+
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
381+
EXPECT_THAT(status, HasErrorMessage("metadata is missing"));
382+
}
383+
384+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDSuccess) {
385+
base_metadata_->default_spec_id = 3;
386+
table::AssertDefaultSpecID requirement(3);
387+
388+
ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk());
389+
}
390+
391+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDMismatch) {
392+
base_metadata_->default_spec_id = 3;
393+
table::AssertDefaultSpecID requirement(7);
394+
395+
auto status = requirement.Validate(base_metadata_.get());
396+
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
397+
EXPECT_THAT(status, HasErrorMessage("spec changed"));
398+
}
399+
400+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDSuccess) {
401+
base_metadata_->default_sort_order_id = 2;
402+
table::AssertDefaultSortOrderID requirement(2);
403+
404+
ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk());
405+
}
406+
407+
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDMismatch) {
408+
base_metadata_->default_sort_order_id = 2;
409+
table::AssertDefaultSortOrderID requirement(4);
410+
411+
auto status = requirement.Validate(base_metadata_.get());
412+
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
413+
EXPECT_THAT(status, HasErrorMessage("sort order changed"));
414+
}
415+
267416
// ============================================================================
268417
// Integration Tests - End-to-End Workflow
269418
// ============================================================================

0 commit comments

Comments
 (0)