Skip to content

Commit 404937c

Browse files
shangxinlishangxinli
committed
Address feedback
1 parent 2902e76 commit 404937c

File tree

2 files changed

+40
-76
lines changed

2 files changed

+40
-76
lines changed

‎src/iceberg/table_requirement.cc‎

Lines changed: 22 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -53,47 +53,35 @@ 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(
64+
"Requirement failed: reference '{}' was created concurrently", 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()) {
70+
} else if (snapshot_id_.has_value()) {
71+
// Reference does not exist but snapshot_id is specified
8072
return CommitFailed(
81-
"Requirement failed: reference '{}' snapshot ID does not match (expected={}, "
82-
"actual={})",
83-
ref_name_, snapshot_id_.value(), it->second->snapshot_id);
73+
"Requirement failed: branch or tag '{}' is missing, expected {}", ref_name_,
74+
snapshot_id_.value());
8475
}
8576

8677
return {};
8778
}
8879

8980
Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const {
9081
// Validate that the last assigned field ID matches the expected value
82+
// Allows base to be null for new tables
9183

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_) {
84+
if (base && base->last_column_id != last_assigned_field_id_) {
9785
return CommitFailed(
9886
"Requirement failed: last assigned field ID does not match (expected={}, "
9987
"actual={})",
@@ -126,12 +114,9 @@ Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const {
126114

127115
Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const {
128116
// Validate that the last assigned partition ID matches the expected value
117+
// Allows base to be null for new tables
129118

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_) {
119+
if (base && base->last_partition_id != last_assigned_partition_id_) {
135120
return CommitFailed(
136121
"Requirement failed: last assigned partition ID does not match (expected={}, "
137122
"actual={})",
@@ -143,14 +128,11 @@ Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const
143128

144129
Status AssertDefaultSpecID::Validate(const TableMetadata* base) const {
145130
// 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-
}
131+
// Matches Java implementation - assumes base is never null
150132

151133
if (base->default_spec_id != spec_id_) {
152134
return CommitFailed(
153-
"Requirement failed: default spec ID does not match (expected={}, actual={})",
135+
"Requirement failed: default partition spec changed: expected id {} != {}",
154136
spec_id_, base->default_spec_id);
155137
}
156138

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

160142
Status AssertDefaultSortOrderID::Validate(const TableMetadata* base) const {
161143
// 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-
}
144+
// Matches Java implementation - assumes base is never null
166145

167146
if (base->default_sort_order_id != sort_order_id_) {
168147
return CommitFailed(
169-
"Requirement failed: default sort order ID does not match (expected={}, "
170-
"actual={})",
148+
"Requirement failed: default sort order changed: expected id {} != {}",
171149
sort_order_id_, base->default_sort_order_id);
172150
}
173151

‎src/iceberg/test/table_metadata_builder_test.cc‎

Lines changed: 18 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -299,24 +299,20 @@ 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 undefined behavior
315+
// This matches Java's assumption that base is never null when Validate is called
320316

321317
TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptSuccess) {
322318
// Ref should not exist, and it doesn't
@@ -336,7 +332,7 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptButEx
336332

337333
auto status = requirement.Validate(base_metadata_.get());
338334
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
339-
EXPECT_THAT(status, HasErrorMessage("should not exist"));
335+
EXPECT_THAT(status, HasErrorMessage("created concurrently"));
340336
}
341337

342338
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdSuccess) {
@@ -358,9 +354,8 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdMismat
358354
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdNullBase) {
359355
table::AssertLastAssignedFieldId requirement(10);
360356

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

366361
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdSuccess) {
@@ -382,9 +377,8 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdMi
382377
TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdNullBase) {
383378
table::AssertLastAssignedPartitionId requirement(5);
384379

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

390384
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDSuccess) {
@@ -400,16 +394,12 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDMismatch) {
400394

401395
auto status = requirement.Validate(base_metadata_.get());
402396
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
403-
EXPECT_THAT(status, HasErrorMessage("default spec ID does not match"));
397+
EXPECT_THAT(status, HasErrorMessage("spec changed"));
404398
}
405399

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-
}
400+
// Removed TableRequirementAssertDefaultSpecIDNullBase test
401+
// Java implementation doesn't check for null base, so passing nullptr would cause undefined behavior
402+
// This matches Java's assumption that base is never null when Validate is called
413403

414404
TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDSuccess) {
415405
base_metadata_->default_sort_order_id = 2;
@@ -424,16 +414,12 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDMismatc
424414

425415
auto status = requirement.Validate(base_metadata_.get());
426416
EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
427-
EXPECT_THAT(status, HasErrorMessage("default sort order ID does not match"));
417+
EXPECT_THAT(status, HasErrorMessage("sort order changed"));
428418
}
429419

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-
}
420+
// Removed TableRequirementAssertDefaultSortOrderIDNullBase test
421+
// Java implementation doesn't check for null base, so passing nullptr would cause undefined behavior
422+
// This matches Java's assumption that base is never null when Validate is called
437423

438424
// ============================================================================
439425
// Integration Tests - End-to-End Workflow

0 commit comments

Comments
 (0)