Skip to content

Commit 793e6df

Browse files
shangxinlishangxinli
committed
fix: address PR feedback to match Java implementations
Changes based on review feedback from @wgtmac: 1. AssertRefSnapshotID: Updated to match Java implementation - Removed null base check (Java doesn't check, assumes base exists) - Updated error messages to match Java version - "was created concurrently" when ref shouldn't exist but does - "has changed: expected id X != Y" when snapshot IDs don't match - "is missing, expected X" when ref should exist but doesn't 2. AssertLastAssignedFieldId: Allow null base metadata - Changed to "if (base && ...)" pattern - Null base is valid for new tables 3. AssertLastAssignedPartitionId: Allow null base metadata - Changed to "if (base && ...)" pattern - Null base is valid for new tables 4. AssertDefaultSpecID: Updated to match Java implementation - Removed null base check (assumes base is never null) - Updated error message: "default partition spec changed: expected id X != Y" 5. AssertDefaultSortOrderID: Updated to match Java implementation - Removed null base check (assumes base is never null) - Updated error message: "default sort order changed: expected id X != Y" All implementations now follow Java patterns from: iceberg-java/core/src/main/java/org/apache/iceberg/UpdateRequirement.java Updated tests to match new behavior: - Updated error message assertions - Removed NullBase tests for methods that don't check null (would cause segfault) - Changed NullBase tests to expect success for methods that allow null base
1 parent e97a034 commit 793e6df

File tree

2 files changed

+43
-77
lines changed

2 files changed

+43
-77
lines changed

‎src/iceberg/table_requirement.cc‎

Lines changed: 22 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -53,47 +53,34 @@ Status AssertUUID::Validate(const TableMetadata* base) const {
5353

5454
Status AssertRefSnapshotID::Validate(const TableMetadata* base) const {
5555
// Validate that a reference (branch or tag) points to the expected snapshot ID
56-
57-
if (base == nullptr) {
58-
return CommitFailed("Requirement failed: current table metadata is missing");
59-
}
56+
// Matches Java implementation logic
6057

6158
auto it = base->refs.find(ref_name_);
62-
63-
// If snapshot_id is nullopt, the reference should not exist
64-
if (!snapshot_id_.has_value()) {
65-
if (it != base->refs.end()) {
59+
if (it != base->refs.end()) {
60+
// Reference exists
61+
if (!snapshot_id_.has_value()) {
62+
// A null snapshot ID means the ref should not exist already
63+
return CommitFailed("Requirement failed: reference '{}' was created concurrently",
64+
ref_name_);
65+
} else if (snapshot_id_.value() != it->second->snapshot_id) {
6666
return CommitFailed(
67-
"Requirement failed: reference '{}' should not exist but found snapshot ID {}",
68-
ref_name_, it->second->snapshot_id);
67+
"Requirement failed: reference '{}' has changed: expected id {} != {}",
68+
ref_name_, snapshot_id_.value(), it->second->snapshot_id);
6969
}
70-
return {};
71-
}
72-
73-
// snapshot_id has a value, so the reference should exist and match
74-
if (it == base->refs.end()) {
75-
return CommitFailed("Requirement failed: reference '{}' is missing in table metadata",
76-
ref_name_);
77-
}
78-
79-
if (it->second->snapshot_id != snapshot_id_.value()) {
80-
return CommitFailed(
81-
"Requirement failed: reference '{}' snapshot ID does not match (expected={}, "
82-
"actual={})",
83-
ref_name_, snapshot_id_.value(), it->second->snapshot_id);
70+
} else if (snapshot_id_.has_value()) {
71+
// Reference does not exist but snapshot_id is specified
72+
return CommitFailed("Requirement failed: branch or tag '{}' is missing, expected {}",
73+
ref_name_, snapshot_id_.value());
8474
}
8575

8676
return {};
8777
}
8878

8979
Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const {
9080
// Validate that the last assigned field ID matches the expected value
81+
// Allows base to be null for new tables
9182

92-
if (base == nullptr) {
93-
return CommitFailed("Requirement failed: current table metadata is missing");
94-
}
95-
96-
if (base->last_column_id != last_assigned_field_id_) {
83+
if (base && base->last_column_id != last_assigned_field_id_) {
9784
return CommitFailed(
9885
"Requirement failed: last assigned field ID does not match (expected={}, "
9986
"actual={})",
@@ -126,12 +113,9 @@ Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const {
126113

127114
Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const {
128115
// Validate that the last assigned partition ID matches the expected value
116+
// Allows base to be null for new tables
129117

130-
if (base == nullptr) {
131-
return CommitFailed("Requirement failed: current table metadata is missing");
132-
}
133-
134-
if (base->last_partition_id != last_assigned_partition_id_) {
118+
if (base && base->last_partition_id != last_assigned_partition_id_) {
135119
return CommitFailed(
136120
"Requirement failed: last assigned partition ID does not match (expected={}, "
137121
"actual={})",
@@ -143,14 +127,11 @@ Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const
143127

144128
Status AssertDefaultSpecID::Validate(const TableMetadata* base) const {
145129
// Validate that the default partition spec ID matches the expected value
146-
147-
if (base == nullptr) {
148-
return CommitFailed("Requirement failed: current table metadata is missing");
149-
}
130+
// Matches Java implementation - assumes base is never null
150131

151132
if (base->default_spec_id != spec_id_) {
152133
return CommitFailed(
153-
"Requirement failed: default spec ID does not match (expected={}, actual={})",
134+
"Requirement failed: default partition spec changed: expected id {} != {}",
154135
spec_id_, base->default_spec_id);
155136
}
156137

@@ -159,15 +140,11 @@ Status AssertDefaultSpecID::Validate(const TableMetadata* base) const {
159140

160141
Status AssertDefaultSortOrderID::Validate(const TableMetadata* base) const {
161142
// Validate that the default sort order ID matches the expected value
162-
163-
if (base == nullptr) {
164-
return CommitFailed("Requirement failed: current table metadata is missing");
165-
}
143+
// Matches Java implementation - assumes base is never null
166144

167145
if (base->default_sort_order_id != sort_order_id_) {
168146
return CommitFailed(
169-
"Requirement failed: default sort order ID does not match (expected={}, "
170-
"actual={})",
147+
"Requirement failed: default sort order changed: expected id {} != {}",
171148
sort_order_id_, base->default_sort_order_id);
172149
}
173150

‎src/iceberg/test/table_metadata_builder_test.cc‎

Lines changed: 21 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -299,24 +299,21 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDMismatch) {
299299

300300
auto status = requirement.Validate(base_metadata_.get());
301301
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
302-
EXPECT_THAT(status, HasErrorMessage("snapshot ID does not match"));
302+
EXPECT_THAT(status, HasErrorMessage("has changed"));
303303
}
304304

305305
TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDRefMissing) {
306306
table::AssertRefSnapshotID requirement("missing-ref", 100);
307307

308308
auto status = requirement.Validate(base_metadata_.get());
309309
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
310-
EXPECT_THAT(status, HasErrorMessage("missing in table metadata"));
310+
EXPECT_THAT(status, HasErrorMessage("is missing"));
311311
}
312312

313-
TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNullBase) {
314-
table::AssertRefSnapshotID requirement("main", 100);
315-
316-
auto status = requirement.Validate(nullptr);
317-
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
318-
EXPECT_THAT(status, HasErrorMessage("metadata is missing"));
319-
}
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
320317

321318
TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptSuccess) {
322319
// Ref should not exist, and it doesn't
@@ -336,7 +333,7 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptButEx
336333

337334
auto status = requirement.Validate(base_metadata_.get());
338335
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
339-
EXPECT_THAT(status, HasErrorMessage("should not exist"));
336+
EXPECT_THAT(status, HasErrorMessage("created concurrently"));
340337
}
341338

342339
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdSuccess) {
@@ -358,9 +355,8 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdMismat
358355
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdNullBase) {
359356
table::AssertLastAssignedFieldId requirement(10);
360357

361-
auto status = requirement.Validate(nullptr);
362-
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
363-
EXPECT_THAT(status, HasErrorMessage("metadata is missing"));
358+
// Null base is now allowed (for new tables) - should succeed
359+
ASSERT_THAT(requirement.Validate(nullptr), IsOk());
364360
}
365361

366362
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdSuccess) {
@@ -382,9 +378,8 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdMi
382378
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdNullBase) {
383379
table::AssertLastAssignedPartitionId requirement(5);
384380

385-
auto status = requirement.Validate(nullptr);
386-
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
387-
EXPECT_THAT(status, HasErrorMessage("metadata is missing"));
381+
// Null base is now allowed (for new tables) - should succeed
382+
ASSERT_THAT(requirement.Validate(nullptr), IsOk());
388383
}
389384

390385
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDSuccess) {
@@ -400,16 +395,13 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDMismatch) {
400395

401396
auto status = requirement.Validate(base_metadata_.get());
402397
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
403-
EXPECT_THAT(status, HasErrorMessage("default spec ID does not match"));
398+
EXPECT_THAT(status, HasErrorMessage("spec changed"));
404399
}
405400

406-
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDNullBase) {
407-
table::AssertDefaultSpecID requirement(3);
408-
409-
auto status = requirement.Validate(nullptr);
410-
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
411-
EXPECT_THAT(status, HasErrorMessage("metadata is missing"));
412-
}
401+
// Removed TableRequirementAssertDefaultSpecIDNullBase test
402+
// Java implementation doesn't check for null base, so passing nullptr would cause
403+
// undefined behavior This matches Java's assumption that base is never null when Validate
404+
// is called
413405

414406
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDSuccess) {
415407
base_metadata_->default_sort_order_id = 2;
@@ -424,16 +416,13 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDMismatc
424416

425417
auto status = requirement.Validate(base_metadata_.get());
426418
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
427-
EXPECT_THAT(status, HasErrorMessage("default sort order ID does not match"));
419+
EXPECT_THAT(status, HasErrorMessage("sort order changed"));
428420
}
429421

430-
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDNullBase) {
431-
table::AssertDefaultSortOrderID requirement(2);
432-
433-
auto status = requirement.Validate(nullptr);
434-
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
435-
EXPECT_THAT(status, HasErrorMessage("metadata is missing"));
436-
}
422+
// Removed TableRequirementAssertDefaultSortOrderIDNullBase test
423+
// Java implementation doesn't check for null base, so passing nullptr would cause
424+
// undefined behavior This matches Java's assumption that base is never null when Validate
425+
// is called
437426

438427
// ============================================================================
439428
// Integration Tests - End-to-End Workflow

0 commit comments

Comments
 (0)