From 56b636841910d89ecf03a32f3456bdac85a33f3f Mon Sep 17 00:00:00 2001 From: Xinli Shang Date: Tue, 4 Nov 2025 08:22:15 -0800 Subject: [PATCH 1/6] feat: implement validation for table update requirements 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. --- src/iceberg/table_requirement.cc | 101 ++++++++++++++++++++++++++++--- 1 file changed, 93 insertions(+), 8 deletions(-) diff --git a/src/iceberg/table_requirement.cc b/src/iceberg/table_requirement.cc index 0b8e79578..668341d39 100644 --- a/src/iceberg/table_requirement.cc +++ b/src/iceberg/table_requirement.cc @@ -19,13 +19,20 @@ #include "iceberg/table_requirement.h" +#include "iceberg/snapshot.h" #include "iceberg/table_metadata.h" #include "iceberg/util/string_util.h" namespace iceberg::table { Status AssertDoesNotExist::Validate(const TableMetadata* base) const { - return NotImplemented("AssertDoesNotExist::Validate not implemented"); + // Validate that the table does not exist + + if (base != nullptr) { + return CommitFailed("Requirement failed: table already exists"); + } + + return {}; } Status AssertUUID::Validate(const TableMetadata* base) const { @@ -45,12 +52,54 @@ Status AssertUUID::Validate(const TableMetadata* base) const { } Status AssertRefSnapshotID::Validate(const TableMetadata* base) const { - return NotImplemented("AssertTableRefSnapshotID::Validate not implemented"); + // Validate that a reference (branch or tag) points to the expected snapshot ID + + if (base == nullptr) { + return CommitFailed("Requirement failed: current table metadata is missing"); + } + + auto it = base->refs.find(ref_name_); + + // If snapshot_id is nullopt, the reference should not exist + if (!snapshot_id_.has_value()) { + if (it != base->refs.end()) { + return CommitFailed( + "Requirement failed: reference '{}' should not exist but found snapshot ID {}", + ref_name_, it->second->snapshot_id); + } + return {}; + } + + // snapshot_id has a value, so the reference should exist and match + if (it == base->refs.end()) { + return CommitFailed( + "Requirement failed: reference '{}' is missing in table metadata", ref_name_); + } + + if (it->second->snapshot_id != snapshot_id_.value()) { + return CommitFailed( + "Requirement failed: reference '{}' snapshot ID does not match (expected={}, " + "actual={})", + ref_name_, snapshot_id_.value(), it->second->snapshot_id); + } + + return {}; } Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const { - return NotImplemented( - "AssertCurrentTableLastAssignedFieldId::Validate not implemented"); + // Validate that the last assigned field ID matches the expected value + + if (base == nullptr) { + return CommitFailed("Requirement failed: current table metadata is missing"); + } + + if (base->last_column_id != last_assigned_field_id_) { + return CommitFailed( + "Requirement failed: last assigned field ID does not match (expected={}, actual={})", + last_assigned_field_id_, base->last_column_id); + } + + return {}; } Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const { @@ -75,16 +124,52 @@ Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const { } Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const { - return NotImplemented( - "AssertCurrentTableLastAssignedPartitionId::Validate not implemented"); + // Validate that the last assigned partition ID matches the expected value + + if (base == nullptr) { + return CommitFailed("Requirement failed: current table metadata is missing"); + } + + if (base->last_partition_id != last_assigned_partition_id_) { + return CommitFailed( + "Requirement failed: last assigned partition ID does not match (expected={}, " + "actual={})", + last_assigned_partition_id_, base->last_partition_id); + } + + return {}; } Status AssertDefaultSpecID::Validate(const TableMetadata* base) const { - return NotImplemented("AssertDefaultTableSpecID::Validate not implemented"); + // Validate that the default partition spec ID matches the expected value + + if (base == nullptr) { + return CommitFailed("Requirement failed: current table metadata is missing"); + } + + if (base->default_spec_id != spec_id_) { + return CommitFailed( + "Requirement failed: default spec ID does not match (expected={}, actual={})", + spec_id_, base->default_spec_id); + } + + return {}; } Status AssertDefaultSortOrderID::Validate(const TableMetadata* base) const { - return NotImplemented("AssertDefaultTableSortOrderID::Validate not implemented"); + // Validate that the default sort order ID matches the expected value + + if (base == nullptr) { + return CommitFailed("Requirement failed: current table metadata is missing"); + } + + if (base->default_sort_order_id != sort_order_id_) { + return CommitFailed( + "Requirement failed: default sort order ID does not match (expected={}, actual={})", + sort_order_id_, base->default_sort_order_id); + } + + return {}; } } // namespace iceberg::table From e97a034e4376b2202e34d3b8e7f896462723b5b0 Mon Sep 17 00:00:00 2001 From: Xinli Shang Date: Tue, 4 Nov 2025 08:29:56 -0800 Subject: [PATCH 2/6] test: add comprehensive tests for table requirement validations Add tests for all newly implemented table requirement validation methods: - AssertDoesNotExist: 2 tests (success, table exists) - AssertRefSnapshotID: 6 tests (success, mismatch, missing ref, null base, nullopt success, nullopt but exists) - AssertLastAssignedFieldId: 3 tests (success, mismatch, null base) - AssertLastAssignedPartitionId: 3 tests (success, mismatch, null base) - AssertDefaultSpecID: 3 tests (success, mismatch, null base) - AssertDefaultSortOrderID: 3 tests (success, mismatch, null base) Total: 20 new tests added. All tests pass (73 tests in table_test suite). --- src/iceberg/table_requirement.cc | 10 +- .../test/table_metadata_builder_test.cc | 171 ++++++++++++++++++ 2 files changed, 177 insertions(+), 4 deletions(-) diff --git a/src/iceberg/table_requirement.cc b/src/iceberg/table_requirement.cc index 668341d39..65ba2e0ea 100644 --- a/src/iceberg/table_requirement.cc +++ b/src/iceberg/table_requirement.cc @@ -72,8 +72,8 @@ Status AssertRefSnapshotID::Validate(const TableMetadata* base) const { // snapshot_id has a value, so the reference should exist and match if (it == base->refs.end()) { - return CommitFailed( - "Requirement failed: reference '{}' is missing in table metadata", ref_name_); + return CommitFailed("Requirement failed: reference '{}' is missing in table metadata", + ref_name_); } if (it->second->snapshot_id != snapshot_id_.value()) { @@ -95,7 +95,8 @@ Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const { if (base->last_column_id != last_assigned_field_id_) { return CommitFailed( - "Requirement failed: last assigned field ID does not match (expected={}, actual={})", + "Requirement failed: last assigned field ID does not match (expected={}, " + "actual={})", last_assigned_field_id_, base->last_column_id); } @@ -165,7 +166,8 @@ Status AssertDefaultSortOrderID::Validate(const TableMetadata* base) const { if (base->default_sort_order_id != sort_order_id_) { return CommitFailed( - "Requirement failed: default sort order ID does not match (expected={}, actual={})", + "Requirement failed: default sort order ID does not match (expected={}, " + "actual={})", sort_order_id_, base->default_sort_order_id); } diff --git a/src/iceberg/test/table_metadata_builder_test.cc b/src/iceberg/test/table_metadata_builder_test.cc index 6b2314f1e..e98a35a93 100644 --- a/src/iceberg/test/table_metadata_builder_test.cc +++ b/src/iceberg/test/table_metadata_builder_test.cc @@ -264,6 +264,177 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertCurrentSchemaIDNotSet) { EXPECT_THAT(status, HasErrorMessage("schema ID is not set")); } +TEST_F(TableMetadataBuilderTest, TableRequirementAssertDoesNotExistSuccess) { + table::AssertDoesNotExist requirement; + + ASSERT_THAT(requirement.Validate(nullptr), IsOk()); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertDoesNotExistTableExists) { + table::AssertDoesNotExist requirement; + + auto status = requirement.Validate(base_metadata_.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("table already exists")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDSuccess) { + auto ref = std::make_shared(); + ref->snapshot_id = 100; + ref->retention = SnapshotRef::Branch{}; + base_metadata_->refs["main"] = ref; + + table::AssertRefSnapshotID requirement("main", 100); + + ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk()); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDMismatch) { + auto ref = std::make_shared(); + ref->snapshot_id = 100; + ref->retention = SnapshotRef::Branch{}; + base_metadata_->refs["main"] = ref; + + table::AssertRefSnapshotID requirement("main", 200); + + auto status = requirement.Validate(base_metadata_.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("snapshot ID does not match")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDRefMissing) { + table::AssertRefSnapshotID requirement("missing-ref", 100); + + auto status = requirement.Validate(base_metadata_.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("missing in table metadata")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNullBase) { + table::AssertRefSnapshotID requirement("main", 100); + + auto status = requirement.Validate(nullptr); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("metadata is missing")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptSuccess) { + // Ref should not exist, and it doesn't + table::AssertRefSnapshotID requirement("nonexistent", std::nullopt); + + ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk()); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptButExists) { + auto ref = std::make_shared(); + ref->snapshot_id = 100; + ref->retention = SnapshotRef::Branch{}; + base_metadata_->refs["main"] = ref; + + // Ref should not exist, but it does + table::AssertRefSnapshotID requirement("main", std::nullopt); + + auto status = requirement.Validate(base_metadata_.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("should not exist")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdSuccess) { + base_metadata_->last_column_id = 10; + table::AssertLastAssignedFieldId requirement(10); + + ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk()); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdMismatch) { + base_metadata_->last_column_id = 10; + table::AssertLastAssignedFieldId requirement(15); + + auto status = requirement.Validate(base_metadata_.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("last assigned field ID does not match")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdNullBase) { + table::AssertLastAssignedFieldId requirement(10); + + auto status = requirement.Validate(nullptr); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("metadata is missing")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdSuccess) { + base_metadata_->last_partition_id = 5; + table::AssertLastAssignedPartitionId requirement(5); + + ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk()); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdMismatch) { + base_metadata_->last_partition_id = 5; + table::AssertLastAssignedPartitionId requirement(8); + + auto status = requirement.Validate(base_metadata_.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("last assigned partition ID does not match")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdNullBase) { + table::AssertLastAssignedPartitionId requirement(5); + + auto status = requirement.Validate(nullptr); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("metadata is missing")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDSuccess) { + base_metadata_->default_spec_id = 3; + table::AssertDefaultSpecID requirement(3); + + ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk()); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDMismatch) { + base_metadata_->default_spec_id = 3; + table::AssertDefaultSpecID requirement(7); + + auto status = requirement.Validate(base_metadata_.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("default spec ID does not match")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDNullBase) { + table::AssertDefaultSpecID requirement(3); + + auto status = requirement.Validate(nullptr); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("metadata is missing")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDSuccess) { + base_metadata_->default_sort_order_id = 2; + table::AssertDefaultSortOrderID requirement(2); + + ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk()); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDMismatch) { + base_metadata_->default_sort_order_id = 2; + table::AssertDefaultSortOrderID requirement(4); + + auto status = requirement.Validate(base_metadata_.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("default sort order ID does not match")); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDNullBase) { + table::AssertDefaultSortOrderID requirement(2); + + auto status = requirement.Validate(nullptr); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("metadata is missing")); +} + // ============================================================================ // Integration Tests - End-to-End Workflow // ============================================================================ From 793e6df457004241e08f5ee2c5269a03f57b0ca2 Mon Sep 17 00:00:00 2001 From: Xinli Shang Date: Mon, 10 Nov 2025 10:20:09 -0800 Subject: [PATCH 3/6] 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 --- src/iceberg/table_requirement.cc | 67 ++++++------------- .../test/table_metadata_builder_test.cc | 53 ++++++--------- 2 files changed, 43 insertions(+), 77 deletions(-) diff --git a/src/iceberg/table_requirement.cc b/src/iceberg/table_requirement.cc index 65ba2e0ea..c3117618f 100644 --- a/src/iceberg/table_requirement.cc +++ b/src/iceberg/table_requirement.cc @@ -53,34 +53,24 @@ Status AssertUUID::Validate(const TableMetadata* base) const { Status AssertRefSnapshotID::Validate(const TableMetadata* base) const { // Validate that a reference (branch or tag) points to the expected snapshot ID - - if (base == nullptr) { - return CommitFailed("Requirement failed: current table metadata is missing"); - } + // Matches Java implementation logic auto it = base->refs.find(ref_name_); - - // If snapshot_id is nullopt, the reference should not exist - if (!snapshot_id_.has_value()) { - if (it != base->refs.end()) { + if (it != base->refs.end()) { + // Reference exists + if (!snapshot_id_.has_value()) { + // A null snapshot ID means the ref should not exist already + return CommitFailed("Requirement failed: reference '{}' was created concurrently", + ref_name_); + } else if (snapshot_id_.value() != it->second->snapshot_id) { return CommitFailed( - "Requirement failed: reference '{}' should not exist but found snapshot ID {}", - ref_name_, it->second->snapshot_id); + "Requirement failed: reference '{}' has changed: expected id {} != {}", + ref_name_, snapshot_id_.value(), it->second->snapshot_id); } - return {}; - } - - // snapshot_id has a value, so the reference should exist and match - if (it == base->refs.end()) { - return CommitFailed("Requirement failed: reference '{}' is missing in table metadata", - ref_name_); - } - - if (it->second->snapshot_id != snapshot_id_.value()) { - return CommitFailed( - "Requirement failed: reference '{}' snapshot ID does not match (expected={}, " - "actual={})", - ref_name_, snapshot_id_.value(), it->second->snapshot_id); + } else if (snapshot_id_.has_value()) { + // Reference does not exist but snapshot_id is specified + return CommitFailed("Requirement failed: branch or tag '{}' is missing, expected {}", + ref_name_, snapshot_id_.value()); } return {}; @@ -88,12 +78,9 @@ Status AssertRefSnapshotID::Validate(const TableMetadata* base) const { Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const { // Validate that the last assigned field ID matches the expected value + // Allows base to be null for new tables - if (base == nullptr) { - return CommitFailed("Requirement failed: current table metadata is missing"); - } - - if (base->last_column_id != last_assigned_field_id_) { + if (base && base->last_column_id != last_assigned_field_id_) { return CommitFailed( "Requirement failed: last assigned field ID does not match (expected={}, " "actual={})", @@ -126,12 +113,9 @@ Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const { Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const { // Validate that the last assigned partition ID matches the expected value + // Allows base to be null for new tables - if (base == nullptr) { - return CommitFailed("Requirement failed: current table metadata is missing"); - } - - if (base->last_partition_id != last_assigned_partition_id_) { + if (base && base->last_partition_id != last_assigned_partition_id_) { return CommitFailed( "Requirement failed: last assigned partition ID does not match (expected={}, " "actual={})", @@ -143,14 +127,11 @@ Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const Status AssertDefaultSpecID::Validate(const TableMetadata* base) const { // Validate that the default partition spec ID matches the expected value - - if (base == nullptr) { - return CommitFailed("Requirement failed: current table metadata is missing"); - } + // Matches Java implementation - assumes base is never null if (base->default_spec_id != spec_id_) { return CommitFailed( - "Requirement failed: default spec ID does not match (expected={}, actual={})", + "Requirement failed: default partition spec changed: expected id {} != {}", spec_id_, base->default_spec_id); } @@ -159,15 +140,11 @@ Status AssertDefaultSpecID::Validate(const TableMetadata* base) const { Status AssertDefaultSortOrderID::Validate(const TableMetadata* base) const { // Validate that the default sort order ID matches the expected value - - if (base == nullptr) { - return CommitFailed("Requirement failed: current table metadata is missing"); - } + // Matches Java implementation - assumes base is never null if (base->default_sort_order_id != sort_order_id_) { return CommitFailed( - "Requirement failed: default sort order ID does not match (expected={}, " - "actual={})", + "Requirement failed: default sort order changed: expected id {} != {}", sort_order_id_, base->default_sort_order_id); } diff --git a/src/iceberg/test/table_metadata_builder_test.cc b/src/iceberg/test/table_metadata_builder_test.cc index e98a35a93..b8d552101 100644 --- a/src/iceberg/test/table_metadata_builder_test.cc +++ b/src/iceberg/test/table_metadata_builder_test.cc @@ -299,7 +299,7 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDMismatch) { auto status = requirement.Validate(base_metadata_.get()); EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); - EXPECT_THAT(status, HasErrorMessage("snapshot ID does not match")); + EXPECT_THAT(status, HasErrorMessage("has changed")); } TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDRefMissing) { @@ -307,16 +307,13 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDRefMissing) auto status = requirement.Validate(base_metadata_.get()); EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); - EXPECT_THAT(status, HasErrorMessage("missing in table metadata")); + EXPECT_THAT(status, HasErrorMessage("is missing")); } -TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNullBase) { - table::AssertRefSnapshotID requirement("main", 100); - - auto status = requirement.Validate(nullptr); - EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); - EXPECT_THAT(status, HasErrorMessage("metadata is missing")); -} +// Removed TableRequirementAssertRefSnapshotIDNullBase test +// Java implementation doesn't check for null base, so passing nullptr would cause +// undefined behavior This matches Java's assumption that base is never null when Validate +// is called TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptSuccess) { // Ref should not exist, and it doesn't @@ -336,7 +333,7 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptButEx auto status = requirement.Validate(base_metadata_.get()); EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); - EXPECT_THAT(status, HasErrorMessage("should not exist")); + EXPECT_THAT(status, HasErrorMessage("created concurrently")); } TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdSuccess) { @@ -358,9 +355,8 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdMismat TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdNullBase) { table::AssertLastAssignedFieldId requirement(10); - auto status = requirement.Validate(nullptr); - EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); - EXPECT_THAT(status, HasErrorMessage("metadata is missing")); + // Null base is now allowed (for new tables) - should succeed + ASSERT_THAT(requirement.Validate(nullptr), IsOk()); } TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdSuccess) { @@ -382,9 +378,8 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdMi TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdNullBase) { table::AssertLastAssignedPartitionId requirement(5); - auto status = requirement.Validate(nullptr); - EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); - EXPECT_THAT(status, HasErrorMessage("metadata is missing")); + // Null base is now allowed (for new tables) - should succeed + ASSERT_THAT(requirement.Validate(nullptr), IsOk()); } TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDSuccess) { @@ -400,16 +395,13 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDMismatch) { auto status = requirement.Validate(base_metadata_.get()); EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); - EXPECT_THAT(status, HasErrorMessage("default spec ID does not match")); + EXPECT_THAT(status, HasErrorMessage("spec changed")); } -TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDNullBase) { - table::AssertDefaultSpecID requirement(3); - - auto status = requirement.Validate(nullptr); - EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); - EXPECT_THAT(status, HasErrorMessage("metadata is missing")); -} +// Removed TableRequirementAssertDefaultSpecIDNullBase test +// Java implementation doesn't check for null base, so passing nullptr would cause +// undefined behavior This matches Java's assumption that base is never null when Validate +// is called TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDSuccess) { base_metadata_->default_sort_order_id = 2; @@ -424,16 +416,13 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDMismatc auto status = requirement.Validate(base_metadata_.get()); EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); - EXPECT_THAT(status, HasErrorMessage("default sort order ID does not match")); + EXPECT_THAT(status, HasErrorMessage("sort order changed")); } -TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDNullBase) { - table::AssertDefaultSortOrderID requirement(2); - - auto status = requirement.Validate(nullptr); - EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); - EXPECT_THAT(status, HasErrorMessage("metadata is missing")); -} +// Removed TableRequirementAssertDefaultSortOrderIDNullBase test +// Java implementation doesn't check for null base, so passing nullptr would cause +// undefined behavior This matches Java's assumption that base is never null when Validate +// is called // ============================================================================ // Integration Tests - End-to-End Workflow From 7ec949c7cb5dd4cdf28b0e07ac61b27a69c95f08 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Tue, 11 Nov 2025 14:50:30 +0800 Subject: [PATCH 4/6] remove AI-generated comments and fix some missing check --- src/iceberg/table_requirement.cc | 66 +++++++++---------- .../test/table_metadata_builder_test.cc | 21 ++---- 2 files changed, 39 insertions(+), 48 deletions(-) diff --git a/src/iceberg/table_requirement.cc b/src/iceberg/table_requirement.cc index c3117618f..98af55719 100644 --- a/src/iceberg/table_requirement.cc +++ b/src/iceberg/table_requirement.cc @@ -21,13 +21,12 @@ #include "iceberg/snapshot.h" #include "iceberg/table_metadata.h" +#include "iceberg/util/macros.h" #include "iceberg/util/string_util.h" namespace iceberg::table { Status AssertDoesNotExist::Validate(const TableMetadata* base) const { - // Validate that the table does not exist - if (base != nullptr) { return CommitFailed("Requirement failed: table already exists"); } @@ -36,39 +35,39 @@ Status AssertDoesNotExist::Validate(const TableMetadata* base) const { } Status AssertUUID::Validate(const TableMetadata* base) const { - // Validate that the table UUID matches the expected value - if (base == nullptr) { return CommitFailed("Requirement failed: current table metadata is missing"); } if (!StringUtils::EqualsIgnoreCase(base->table_uuid, uuid_)) { return CommitFailed( - "Requirement failed: table UUID does not match (expected='{}', actual='{}')", - uuid_, base->table_uuid); + "Requirement failed: table UUID does not match, expected {} != {}", uuid_, + base->table_uuid); } return {}; } Status AssertRefSnapshotID::Validate(const TableMetadata* base) const { - // Validate that a reference (branch or tag) points to the expected snapshot ID - // Matches Java implementation logic + if (base == nullptr) { + return CommitFailed("Requirement failed: current table metadata is missing"); + } - auto it = base->refs.find(ref_name_); - if (it != base->refs.end()) { - // Reference exists + if (auto ref = base->refs.find(ref_name_); ref != base->refs.cend()) { + const auto& snapshot_ref = ref->second; + ICEBERG_DCHECK(snapshot_ref != nullptr, "Snapshot reference is null"); + std::string_view type = + snapshot_ref->type() == SnapshotRefType::kBranch ? "branch" : "tag"; if (!snapshot_id_.has_value()) { // A null snapshot ID means the ref should not exist already - return CommitFailed("Requirement failed: reference '{}' was created concurrently", + return CommitFailed("Requirement failed: {} '{}' was created concurrently", type, ref_name_); - } else if (snapshot_id_.value() != it->second->snapshot_id) { - return CommitFailed( - "Requirement failed: reference '{}' has changed: expected id {} != {}", - ref_name_, snapshot_id_.value(), it->second->snapshot_id); + } else if (snapshot_id_.value() != snapshot_ref->snapshot_id) { + return CommitFailed("Requirement failed: {} '{}' has changed: expected id {} != {}", + type, ref_name_, snapshot_id_.value(), + snapshot_ref->snapshot_id); } } else if (snapshot_id_.has_value()) { - // Reference does not exist but snapshot_id is specified return CommitFailed("Requirement failed: branch or tag '{}' is missing, expected {}", ref_name_, snapshot_id_.value()); } @@ -77,13 +76,13 @@ Status AssertRefSnapshotID::Validate(const TableMetadata* base) const { } Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const { - // Validate that the last assigned field ID matches the expected value - // Allows base to be null for new tables + if (base == nullptr) { + return CommitFailed("Requirement failed: current table metadata is missing"); + } if (base && base->last_column_id != last_assigned_field_id_) { return CommitFailed( - "Requirement failed: last assigned field ID does not match (expected={}, " - "actual={})", + "Requirement failed: last assigned field ID does not match, expected {} != {}", last_assigned_field_id_, base->last_column_id); } @@ -91,8 +90,6 @@ Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const { } Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const { - // Validate that the current schema ID matches the one used when the metadata was read - if (base == nullptr) { return CommitFailed("Requirement failed: current table metadata is missing"); } @@ -104,7 +101,7 @@ Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const { if (base->current_schema_id.value() != schema_id_) { return CommitFailed( - "Requirement failed: current schema ID does not match (expected={}, actual={})", + "Requirement failed: current schema ID does not match, expected {} != {}", schema_id_, base->current_schema_id.value()); } @@ -112,13 +109,14 @@ Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const { } Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const { - // Validate that the last assigned partition ID matches the expected value - // Allows base to be null for new tables + if (base == nullptr) { + return CommitFailed("Requirement failed: current table metadata is missing"); + } if (base && base->last_partition_id != last_assigned_partition_id_) { return CommitFailed( - "Requirement failed: last assigned partition ID does not match (expected={}, " - "actual={})", + "Requirement failed: last assigned partition ID does not match, expected {} != " + "{}", last_assigned_partition_id_, base->last_partition_id); } @@ -126,12 +124,13 @@ Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const } Status AssertDefaultSpecID::Validate(const TableMetadata* base) const { - // Validate that the default partition spec ID matches the expected value - // Matches Java implementation - assumes base is never null + if (base == nullptr) { + return CommitFailed("Requirement failed: current table metadata is missing"); + } if (base->default_spec_id != spec_id_) { return CommitFailed( - "Requirement failed: default partition spec changed: expected id {} != {}", + "Requirement failed: default partition spec changed, expected id {} != {}", spec_id_, base->default_spec_id); } @@ -139,8 +138,9 @@ Status AssertDefaultSpecID::Validate(const TableMetadata* base) const { } Status AssertDefaultSortOrderID::Validate(const TableMetadata* base) const { - // Validate that the default sort order ID matches the expected value - // Matches Java implementation - assumes base is never null + if (base == nullptr) { + return CommitFailed("Requirement failed: current table metadata is missing"); + } if (base->default_sort_order_id != sort_order_id_) { return CommitFailed( diff --git a/src/iceberg/test/table_metadata_builder_test.cc b/src/iceberg/test/table_metadata_builder_test.cc index b8d552101..3a0724e37 100644 --- a/src/iceberg/test/table_metadata_builder_test.cc +++ b/src/iceberg/test/table_metadata_builder_test.cc @@ -328,7 +328,6 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptButEx ref->retention = SnapshotRef::Branch{}; base_metadata_->refs["main"] = ref; - // Ref should not exist, but it does table::AssertRefSnapshotID requirement("main", std::nullopt); auto status = requirement.Validate(base_metadata_.get()); @@ -355,8 +354,9 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdMismat TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdNullBase) { table::AssertLastAssignedFieldId requirement(10); - // Null base is now allowed (for new tables) - should succeed - ASSERT_THAT(requirement.Validate(nullptr), IsOk()); + auto status = requirement.Validate(nullptr); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("metadata is missing")); } TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdSuccess) { @@ -378,8 +378,9 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdMi TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdNullBase) { table::AssertLastAssignedPartitionId requirement(5); - // Null base is now allowed (for new tables) - should succeed - ASSERT_THAT(requirement.Validate(nullptr), IsOk()); + auto status = requirement.Validate(nullptr); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("metadata is missing")); } TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDSuccess) { @@ -398,11 +399,6 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDMismatch) { EXPECT_THAT(status, HasErrorMessage("spec changed")); } -// Removed TableRequirementAssertDefaultSpecIDNullBase test -// Java implementation doesn't check for null base, so passing nullptr would cause -// undefined behavior This matches Java's assumption that base is never null when Validate -// is called - TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDSuccess) { base_metadata_->default_sort_order_id = 2; table::AssertDefaultSortOrderID requirement(2); @@ -419,11 +415,6 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDMismatc EXPECT_THAT(status, HasErrorMessage("sort order changed")); } -// Removed TableRequirementAssertDefaultSortOrderIDNullBase test -// Java implementation doesn't check for null base, so passing nullptr would cause -// undefined behavior This matches Java's assumption that base is never null when Validate -// is called - // ============================================================================ // Integration Tests - End-to-End Workflow // ============================================================================ From 45e0d234005ba4fbb7c38528afcd4c77eb8db0d1 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Tue, 11 Nov 2025 15:43:01 +0800 Subject: [PATCH 5/6] Update src/iceberg/table_requirement.cc --- src/iceberg/table_requirement.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/iceberg/table_requirement.cc b/src/iceberg/table_requirement.cc index 98af55719..3c0a01e48 100644 --- a/src/iceberg/table_requirement.cc +++ b/src/iceberg/table_requirement.cc @@ -76,10 +76,6 @@ Status AssertRefSnapshotID::Validate(const TableMetadata* base) const { } Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const { - if (base == nullptr) { - return CommitFailed("Requirement failed: current table metadata is missing"); - } - if (base && base->last_column_id != last_assigned_field_id_) { return CommitFailed( "Requirement failed: last assigned field ID does not match, expected {} != {}", From 6a6da570d39c478f29593369663d214211521300 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Tue, 11 Nov 2025 17:52:56 +0800 Subject: [PATCH 6/6] fix test --- src/iceberg/test/table_metadata_builder_test.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/iceberg/test/table_metadata_builder_test.cc b/src/iceberg/test/table_metadata_builder_test.cc index 3a0724e37..9b9804819 100644 --- a/src/iceberg/test/table_metadata_builder_test.cc +++ b/src/iceberg/test/table_metadata_builder_test.cc @@ -354,9 +354,7 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdMismat TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdNullBase) { table::AssertLastAssignedFieldId requirement(10); - auto status = requirement.Validate(nullptr); - EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); - EXPECT_THAT(status, HasErrorMessage("metadata is missing")); + EXPECT_THAT(requirement.Validate(nullptr), IsOk()); } TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdSuccess) {