From d27c8f4c78f997c462be942dcfc263711488a737 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Wed, 3 Dec 2025 15:29:51 +0800 Subject: [PATCH 1/6] 1 --- src/iceberg/snapshot.h | 2 + src/iceberg/table_requirements.cc | 68 ++++- src/iceberg/table_requirements.h | 34 +-- src/iceberg/table_update.cc | 78 +++--- src/iceberg/test/table_update_test.cc | 373 +++++++++++++++++++++++++- src/iceberg/transaction.h | 1 - 6 files changed, 480 insertions(+), 76 deletions(-) diff --git a/src/iceberg/snapshot.h b/src/iceberg/snapshot.h index d41795d53..5afe2d22e 100644 --- a/src/iceberg/snapshot.h +++ b/src/iceberg/snapshot.h @@ -62,6 +62,8 @@ ICEBERG_EXPORT constexpr Result SnapshotRefTypeFromString( /// \brief A reference to a snapshot, either a branch or a tag. struct ICEBERG_EXPORT SnapshotRef { + static constexpr std::string_view kMainBranch = "main"; + struct ICEBERG_EXPORT Branch { /// A positive number for the minimum number of snapshots to keep in a branch while /// expiring snapshots. Defaults to table property diff --git a/src/iceberg/table_requirements.cc b/src/iceberg/table_requirements.cc index 3e0aa024f..d0bc86666 100644 --- a/src/iceberg/table_requirements.cc +++ b/src/iceberg/table_requirements.cc @@ -21,6 +21,7 @@ #include +#include "iceberg/snapshot.h" #include "iceberg/table_metadata.h" #include "iceberg/table_requirement.h" #include "iceberg/table_update.h" @@ -36,12 +37,73 @@ Result>> TableUpdateContext::Build return std::move(requirements_); } +void TableUpdateContext::RequireLastAssignedFieldIdUnchanged() { + if (!added_last_assigned_field_id_) { + if (base_ != nullptr) { + AddRequirement( + std::make_unique(base_->last_column_id)); + } + added_last_assigned_field_id_ = true; + } +} + +void TableUpdateContext::RequireCurrentSchemaIdUnchanged() { + if (!added_current_schema_id_) { + if (base_ != nullptr && !is_replace_) { + AddRequirement(std::make_unique( + base_->current_schema_id.value())); + } + added_current_schema_id_ = true; + } +} + +void TableUpdateContext::RequireLastAssignedPartitionIdUnchanged() { + if (!added_last_assigned_partition_id_) { + if (base_ != nullptr) { + AddRequirement(std::make_unique( + base_->last_partition_id)); + } + added_last_assigned_partition_id_ = true; + } +} + +void TableUpdateContext::RequireDefaultSpecIdUnchanged() { + if (!added_default_spec_id_) { + if (base_ != nullptr && !is_replace_) { + AddRequirement( + std::make_unique(base_->default_spec_id)); + } + added_default_spec_id_ = true; + } +} + +void TableUpdateContext::RequireDefaultSortOrderIdUnchanged() { + if (!added_default_sort_order_id_) { + if (base_ != nullptr && !is_replace_) { + AddRequirement(std::make_unique( + base_->default_sort_order_id)); + } + added_default_sort_order_id_ = true; + } +} + +void TableUpdateContext::RequireNoBranchesChanged() { + if (base_ != nullptr && !is_replace_) { + for (const auto& [name, ref] : base_->refs) { + if (ref->type() == SnapshotRefType::kBranch && name != SnapshotRef::kMainBranch) { + AddRequirement( + std::make_unique(name, ref->snapshot_id)); + } + } + } +} + Result>> TableRequirements::ForCreateTable( const std::vector>& table_updates) { TableUpdateContext context(nullptr, false); context.AddRequirement(std::make_unique()); for (const auto& update : table_updates) { - ICEBERG_RETURN_UNEXPECTED(update->GenerateRequirements(context)); + update->GenerateRequirements(context); } return context.Build(); } @@ -52,7 +114,7 @@ Result>> TableRequirements::ForRep TableUpdateContext context(&base, true); context.AddRequirement(std::make_unique(base.table_uuid)); for (const auto& update : table_updates) { - ICEBERG_RETURN_UNEXPECTED(update->GenerateRequirements(context)); + update->GenerateRequirements(context); } return context.Build(); } @@ -63,7 +125,7 @@ Result>> TableRequirements::ForUpd TableUpdateContext context(&base, false); context.AddRequirement(std::make_unique(base.table_uuid)); for (const auto& update : table_updates) { - ICEBERG_RETURN_UNEXPECTED(update->GenerateRequirements(context)); + update->GenerateRequirements(context); } return context.Build(); } diff --git a/src/iceberg/table_requirements.h b/src/iceberg/table_requirements.h index 7af2fb2df..7e5c0e74f 100644 --- a/src/iceberg/table_requirements.h +++ b/src/iceberg/table_requirements.h @@ -68,27 +68,19 @@ class ICEBERG_EXPORT TableUpdateContext { /// \brief Build and return the list of requirements Result>> Build(); - // Getters for deduplication flags - bool added_last_assigned_field_id() const { return added_last_assigned_field_id_; } - bool added_current_schema_id() const { return added_current_schema_id_; } - bool added_last_assigned_partition_id() const { - return added_last_assigned_partition_id_; - } - bool added_default_spec_id() const { return added_default_spec_id_; } - bool added_default_sort_order_id() const { return added_default_sort_order_id_; } - - // Setters for deduplication flags - void set_added_last_assigned_field_id(bool value) { - added_last_assigned_field_id_ = value; - } - void set_added_current_schema_id(bool value) { added_current_schema_id_ = value; } - void set_added_last_assigned_partition_id(bool value) { - added_last_assigned_partition_id_ = value; - } - void set_added_default_spec_id(bool value) { added_default_spec_id_ = value; } - void set_added_default_sort_order_id(bool value) { - added_default_sort_order_id_ = value; - } + // Helper methods to deduplicate requirements to add. + /// \brief Require that the last assigned field ID remains unchanged + void RequireLastAssignedFieldIdUnchanged(); + /// \brief Require that the current schema ID remains unchanged + void RequireCurrentSchemaIdUnchanged(); + /// \brief Require that the last assigned partition ID remains unchanged + void RequireLastAssignedPartitionIdUnchanged(); + /// \brief Require that the default spec ID remains unchanged + void RequireDefaultSpecIdUnchanged(); + /// \brief Require that the default sort order ID remains unchanged + void RequireDefaultSortOrderIdUnchanged(); + /// \brief Require that no branches have been changed + void RequireNoBranchesChanged(); private: const TableMetadata* base_; diff --git a/src/iceberg/table_update.cc b/src/iceberg/table_update.cc index 6c1ad72e4..2e1c82783 100644 --- a/src/iceberg/table_update.cc +++ b/src/iceberg/table_update.cc @@ -21,7 +21,6 @@ #include "iceberg/exception.h" #include "iceberg/table_metadata.h" -#include "iceberg/table_requirement.h" #include "iceberg/table_requirements.h" namespace iceberg::table { @@ -32,9 +31,8 @@ void AssignUUID::ApplyTo(TableMetadataBuilder& builder) const { builder.AssignUUID(uuid_); } -Status AssignUUID::GenerateRequirements(TableUpdateContext& context) const { +void AssignUUID::GenerateRequirements(TableUpdateContext& context) const { // AssignUUID does not generate additional requirements. - return {}; } // UpgradeFormatVersion @@ -43,8 +41,8 @@ void UpgradeFormatVersion::ApplyTo(TableMetadataBuilder& builder) const { throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -Status UpgradeFormatVersion::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("UpgradeFormatVersion::GenerateRequirements not implemented"); +void UpgradeFormatVersion::GenerateRequirements(TableUpdateContext& context) const { + // UpgradeFormatVersion doesn't generate any requirements } // AddSchema @@ -53,8 +51,8 @@ void AddSchema::ApplyTo(TableMetadataBuilder& builder) const { throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -Status AddSchema::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("AddTableSchema::GenerateRequirements not implemented"); +void AddSchema::GenerateRequirements(TableUpdateContext& context) const { + context.RequireLastAssignedFieldIdUnchanged(); } // SetCurrentSchema @@ -63,8 +61,8 @@ void SetCurrentSchema::ApplyTo(TableMetadataBuilder& builder) const { throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -Status SetCurrentSchema::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("SetCurrentTableSchema::GenerateRequirements not implemented"); +void SetCurrentSchema::GenerateRequirements(TableUpdateContext& context) const { + context.RequireCurrentSchemaIdUnchanged(); } // AddPartitionSpec @@ -73,8 +71,8 @@ void AddPartitionSpec::ApplyTo(TableMetadataBuilder& builder) const { throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -Status AddPartitionSpec::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("AddTablePartitionSpec::GenerateRequirements not implemented"); +void AddPartitionSpec::GenerateRequirements(TableUpdateContext& context) const { + context.RequireLastAssignedPartitionIdUnchanged(); } // SetDefaultPartitionSpec @@ -83,9 +81,8 @@ void SetDefaultPartitionSpec::ApplyTo(TableMetadataBuilder& builder) const { throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -Status SetDefaultPartitionSpec::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented( - "SetDefaultTablePartitionSpec::GenerateRequirements not implemented"); +void SetDefaultPartitionSpec::GenerateRequirements(TableUpdateContext& context) const { + context.RequireDefaultSpecIdUnchanged(); } // RemovePartitionSpecs @@ -94,9 +91,9 @@ void RemovePartitionSpecs::ApplyTo(TableMetadataBuilder& builder) const { throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -Status RemovePartitionSpecs::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented( - "RemoveTablePartitionSpecs::GenerateRequirements not implemented"); +void RemovePartitionSpecs::GenerateRequirements(TableUpdateContext& context) const { + context.RequireDefaultSpecIdUnchanged(); + context.RequireNoBranchesChanged(); } // RemoveSchemas @@ -105,28 +102,29 @@ void RemoveSchemas::ApplyTo(TableMetadataBuilder& builder) const { throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -Status RemoveSchemas::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("RemoveTableSchemas::GenerateRequirements not implemented"); +void RemoveSchemas::GenerateRequirements(TableUpdateContext& context) const { + context.RequireCurrentSchemaIdUnchanged(); + context.RequireNoBranchesChanged(); } // AddSortOrder void AddSortOrder::ApplyTo(TableMetadataBuilder& builder) const { - throw IcebergError(std::format("{} not implemented", __FUNCTION__)); + builder.AddSortOrder(sort_order_); } -Status AddSortOrder::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("AddTableSortOrder::GenerateRequirements not implemented"); +void AddSortOrder::GenerateRequirements(TableUpdateContext& context) const { + // AddSortOrder doesn't generate any requirements } // SetDefaultSortOrder void SetDefaultSortOrder::ApplyTo(TableMetadataBuilder& builder) const { - throw IcebergError(std::format("{} not implemented", __FUNCTION__)); + builder.SetDefaultSortOrder(sort_order_id_); } -Status SetDefaultSortOrder::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("SetDefaultTableSortOrder::GenerateRequirements not implemented"); +void SetDefaultSortOrder::GenerateRequirements(TableUpdateContext& context) const { + context.RequireDefaultSortOrderIdUnchanged(); } // AddSnapshot @@ -135,16 +133,16 @@ void AddSnapshot::ApplyTo(TableMetadataBuilder& builder) const { throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -Status AddSnapshot::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("AddTableSnapshot::GenerateRequirements not implemented"); +void AddSnapshot::GenerateRequirements(TableUpdateContext& context) const { + // AddSnapshot doesn't generate any requirements } // RemoveSnapshots void RemoveSnapshots::ApplyTo(TableMetadataBuilder& builder) const {} -Status RemoveSnapshots::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("RemoveTableSnapshots::GenerateRequirements not implemented"); +void RemoveSnapshots::GenerateRequirements(TableUpdateContext& context) const { + throw IcebergError("RemoveTableSnapshots::GenerateRequirements not implemented"); } // RemoveSnapshotRef @@ -153,8 +151,8 @@ void RemoveSnapshotRef::ApplyTo(TableMetadataBuilder& builder) const { throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -Status RemoveSnapshotRef::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("RemoveTableSnapshotRef::GenerateRequirements not implemented"); +void RemoveSnapshotRef::GenerateRequirements(TableUpdateContext& context) const { + // RemoveSnapshotRef doesn't generate any requirements } // SetSnapshotRef @@ -163,8 +161,8 @@ void SetSnapshotRef::ApplyTo(TableMetadataBuilder& builder) const { throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -Status SetSnapshotRef::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("SetTableSnapshotRef::GenerateRequirements not implemented"); +void SetSnapshotRef::GenerateRequirements(TableUpdateContext& context) const { + throw NotImplemented("SetTableSnapshotRef::GenerateRequirements not implemented"); } // SetProperties @@ -173,9 +171,8 @@ void SetProperties::ApplyTo(TableMetadataBuilder& builder) const { builder.SetProperties(updated_); } -Status SetProperties::GenerateRequirements(TableUpdateContext& context) const { - // No requirements - return {}; +void SetProperties::GenerateRequirements(TableUpdateContext& context) const { + // SetProperties doesn't generate any requirements } // RemoveProperties @@ -184,9 +181,8 @@ void RemoveProperties::ApplyTo(TableMetadataBuilder& builder) const { builder.RemoveProperties(removed_); } -Status RemoveProperties::GenerateRequirements(TableUpdateContext& context) const { - // No requirements - return {}; +void RemoveProperties::GenerateRequirements(TableUpdateContext& context) const { + // RemoveProperties doesn't generate any requirements } // SetLocation @@ -195,8 +191,8 @@ void SetLocation::ApplyTo(TableMetadataBuilder& builder) const { throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -Status SetLocation::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("SetTableLocation::GenerateRequirements not implemented"); +void SetLocation::GenerateRequirements(TableUpdateContext& context) const { + // SetLocation doesn't generate any requirements } } // namespace iceberg::table diff --git a/src/iceberg/test/table_update_test.cc b/src/iceberg/test/table_update_test.cc index 298141a93..e7d725d57 100644 --- a/src/iceberg/test/table_update_test.cc +++ b/src/iceberg/test/table_update_test.cc @@ -26,23 +26,34 @@ #include #include "iceberg/partition_spec.h" +#include "iceberg/schema.h" #include "iceberg/snapshot.h" +#include "iceberg/sort_field.h" #include "iceberg/sort_order.h" #include "iceberg/table_metadata.h" #include "iceberg/table_requirement.h" #include "iceberg/table_requirements.h" #include "iceberg/test/matchers.h" +#include "iceberg/transform.h" +#include "iceberg/type.h" namespace iceberg { namespace { +// Helper function to create a simple schema for testing +std::shared_ptr CreateTestSchema() { + auto field1 = SchemaField::MakeRequired(1, "id", int32()); + auto field2 = SchemaField::MakeRequired(2, "data", string()); + auto field3 = SchemaField::MakeRequired(3, "ts", timestamp()); + return std::make_shared(std::vector{field1, field2, field3}, 0); +} + // Helper function to generate requirements std::vector> GenerateRequirements( const TableUpdate& update, const TableMetadata* base) { TableUpdateContext context(base, /*is_replace=*/false); - EXPECT_THAT(update.GenerateRequirements(context), IsOk()); - + update.GenerateRequirements(context), IsOk(); auto requirements = context.Build(); EXPECT_THAT(requirements, IsOk()); return std::move(requirements.value()); @@ -56,18 +67,33 @@ std::unique_ptr CreateBaseMetadata() { metadata->location = "s3://bucket/test"; metadata->last_sequence_number = 0; metadata->last_updated_ms = TimePointMs{std::chrono::milliseconds(1000)}; - metadata->last_column_id = 0; + metadata->last_column_id = 3; + metadata->current_schema_id = 0; + metadata->schemas.push_back(CreateTestSchema()); metadata->default_spec_id = PartitionSpec::kInitialSpecId; metadata->last_partition_id = 0; metadata->current_snapshot_id = Snapshot::kInvalidSnapshotId; metadata->default_sort_order_id = SortOrder::kInitialSortOrderId; + metadata->sort_orders.push_back(SortOrder::Unsorted()); metadata->next_row_id = TableMetadata::kInitialRowId; return metadata; } } // namespace -// Test GenerateRequirements for AssignUUID update +// Test AssignUUID +TEST(TableUpdateTest, AssignUUIDApplyUpdate) { + auto base = CreateBaseMetadata(); + auto builder = TableMetadataBuilder::BuildFrom(base.get()); + + // Apply AssignUUID update + table::AssignUUID uuid_update("apply-uuid"); + uuid_update.ApplyTo(*builder); + + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); + EXPECT_EQ(metadata->table_uuid, "apply-uuid"); +} + TEST(TableUpdateTest, AssignUUIDGenerateRequirements) { table::AssignUUID update("new-uuid"); @@ -75,16 +101,343 @@ TEST(TableUpdateTest, AssignUUIDGenerateRequirements) { auto new_table_reqs = GenerateRequirements(update, nullptr); EXPECT_TRUE(new_table_reqs.empty()); - // Existing table - AssignUUID doesn't generate requirements anymore - // The UUID assertion is added by ForUpdateTable/ForReplaceTable methods + // Existing table - no requirements + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + EXPECT_TRUE(existing_table_reqs.empty()); +} + +// Test UpgradeFormatVersion +TEST(TableUpdateTest, UpgradeFormatVersionGenerateRequirements) { + table::UpgradeFormatVersion update(3); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - no requirements + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + EXPECT_TRUE(existing_table_reqs.empty()); +} + +// Test AddSchema +TEST(TableUpdateTest, AddSchemaGenerateRequirements) { + auto new_schema = std::make_shared( + std::vector{SchemaField::MakeRequired(4, "new_col", string())}, 3); + table::AddSchema update(new_schema, 3); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - generates AssertLastAssignedFieldId requirement + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + ASSERT_EQ(existing_table_reqs.size(), 1); + + auto* assert_id = + dynamic_cast(existing_table_reqs[0].get()); + ASSERT_NE(assert_id, nullptr); + EXPECT_EQ(assert_id->last_assigned_field_id(), base->last_column_id); +} + +// Test SetCurrentSchema +TEST(TableUpdateTest, SetCurrentSchemaGenerateRequirements) { + table::SetCurrentSchema update(1); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - generates AssertCurrentSchemaID requirement + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + ASSERT_EQ(existing_table_reqs.size(), 1); + + auto* assert_id = + dynamic_cast(existing_table_reqs[0].get()); + ASSERT_NE(assert_id, nullptr); + EXPECT_EQ(assert_id->schema_id(), base->current_schema_id); +} + +// Test AddPartitionSpec +TEST(TableUpdateTest, AddPartitionSpecGenerateRequirements) { + PartitionField partition_field(1, 1, "id_identity", Transform::Identity()); + ICEBERG_UNWRAP_OR_FAIL(auto spec_unique, PartitionSpec::Make(1, {partition_field})); + auto spec = std::shared_ptr(std::move(spec_unique)); + table::AddPartitionSpec update(spec); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - generates AssertLastAssignedPartitionId requirement + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + ASSERT_EQ(existing_table_reqs.size(), 1); + + auto* assert_id = + dynamic_cast(existing_table_reqs[0].get()); + ASSERT_NE(assert_id, nullptr); + EXPECT_EQ(assert_id->last_assigned_partition_id(), base->last_partition_id); +} + +// Test SetDefaultPartitionSpec +TEST(TableUpdateTest, SetDefaultPartitionSpecGenerateRequirements) { + table::SetDefaultPartitionSpec update(1); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - generates AssertDefaultSpecID requirement + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + ASSERT_EQ(existing_table_reqs.size(), 1); + + auto* assert_id = + dynamic_cast(existing_table_reqs[0].get()); + ASSERT_NE(assert_id, nullptr); + EXPECT_EQ(assert_id->spec_id(), base->default_spec_id); +} + +// Test RemovePartitionSpecs +TEST(TableUpdateTest, RemovePartitionSpecsGenerateRequirements) { + table::RemovePartitionSpecs update(std::vector{1}); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table without snapshot - one requirement + auto base = CreateBaseMetadata(); + auto existing_table_reqs_no_snapshot = GenerateRequirements(update, base.get()); + ASSERT_EQ(existing_table_reqs_no_snapshot.size(), 1); + auto* assert_spec_id = + dynamic_cast(existing_table_reqs_no_snapshot[0].get()); + ASSERT_NE(assert_spec_id, nullptr); + EXPECT_EQ(assert_spec_id->spec_id(), base->default_spec_id); + + // Existing table with a non-main branch - two requirements + base->current_snapshot_id = 12345; + auto snapshot = std::make_shared(); + snapshot->snapshot_id = 12345; + snapshot->sequence_number = 1; + base->snapshots.push_back(snapshot); + const std::string dev_branch = "dev"; + base->refs.emplace(dev_branch, + std::make_shared(SnapshotRef{ + .snapshot_id = 12345, .retention = SnapshotRef::Branch{}})); + + auto existing_table_reqs_with_snapshot = GenerateRequirements(update, base.get()); + ASSERT_EQ(existing_table_reqs_with_snapshot.size(), 2); + + auto* req1 = + dynamic_cast(existing_table_reqs_no_snapshot[0].get()); + EXPECT_EQ(req1->spec_id(), base->default_spec_id); + auto* req2 = dynamic_cast( + existing_table_reqs_with_snapshot[1].get()); + EXPECT_EQ(req2->ref_name(), dev_branch); + EXPECT_TRUE(req2->snapshot_id().has_value()); + EXPECT_EQ(req2->snapshot_id().value(), base->current_snapshot_id); +} + +// Test RemoveSchemas +TEST(TableUpdateTest, RemoveSchemasGenerateRequirements) { + table::RemoveSchemas update({1}); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table without snapshot - one requirement + auto base = CreateBaseMetadata(); + auto existing_table_reqs_no_snapshot = GenerateRequirements(update, base.get()); + ASSERT_EQ(existing_table_reqs_no_snapshot.size(), 1); + auto* assert_schema_id = dynamic_cast( + existing_table_reqs_no_snapshot[0].get()); + ASSERT_NE(assert_schema_id, nullptr); + EXPECT_EQ(assert_schema_id->schema_id(), base->current_schema_id); + + // Existing table with a non-main branch - two requirements + base->current_snapshot_id = 12345; + auto snapshot = std::make_shared(); + snapshot->snapshot_id = 12345; + snapshot->sequence_number = 1; + base->snapshots.push_back(snapshot); + const std::string dev_branch = "dev"; + base->refs.emplace(dev_branch, + std::make_shared(SnapshotRef{ + .snapshot_id = 12345, .retention = SnapshotRef::Branch{}})); + + auto existing_table_reqs_with_snapshot = GenerateRequirements(update, base.get()); + ASSERT_EQ(existing_table_reqs_with_snapshot.size(), 2); + + // The order is not guaranteed. + bool found_schema_id_req = false; + bool found_branch_req = false; + for (const auto& req : existing_table_reqs_with_snapshot) { + if (auto* r = dynamic_cast(req.get())) { + EXPECT_EQ(r->schema_id(), base->current_schema_id); + found_schema_id_req = true; + } else if (auto* r = dynamic_cast(req.get())) { + EXPECT_EQ(r->ref_name(), dev_branch); + EXPECT_TRUE(r->snapshot_id().has_value()); + EXPECT_EQ(r->snapshot_id().value(), base->current_snapshot_id); + found_branch_req = true; + } + } + EXPECT_TRUE(found_schema_id_req); + EXPECT_TRUE(found_branch_req); +} + +// Test AddSortOrder +TEST(TableUpdateTest, ApplyAddSortOrderUpdate) { + auto base = CreateBaseMetadata(); + auto builder = TableMetadataBuilder::BuildFrom(base.get()); + + // Apply AddSortOrder update + auto schema = CreateTestSchema(); + SortField sort_field(1, Transform::Identity(), SortDirection::kAscending, + NullOrder::kFirst); + ICEBERG_UNWRAP_OR_FAIL(auto sort_order_unique, + SortOrder::Make(*schema, 1, std::vector{sort_field})); + auto sort_order = std::shared_ptr(std::move(sort_order_unique)); + table::AddSortOrder sort_order_update(sort_order); + sort_order_update.ApplyTo(*builder); + + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); + ASSERT_EQ(metadata->sort_orders.size(), 2); + EXPECT_EQ(metadata->sort_orders[1]->order_id(), 1); +} + +TEST(TableUpdateTest, AddSortOrderGenerateRequirements) { + auto schema = CreateTestSchema(); + SortField sort_field(1, Transform::Identity(), SortDirection::kAscending, + NullOrder::kFirst); + ICEBERG_UNWRAP_OR_FAIL(std::shared_ptr sort_order, + SortOrder::Make(*schema, 1, std::vector{sort_field})); + table::AddSortOrder update(sort_order); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - no requirements auto base = CreateBaseMetadata(); auto existing_table_reqs = GenerateRequirements(update, base.get()); EXPECT_TRUE(existing_table_reqs.empty()); +} + +// Test SetDefaultSortOrder +TEST(TableUpdateTest, ApplySetDefaultSortOrderUpdate) { + auto base = CreateBaseMetadata(); + auto builder = TableMetadataBuilder::BuildFrom(base.get()); + + auto schema = CreateTestSchema(); + SortField sort_field(1, Transform::Identity(), SortDirection::kAscending, + NullOrder::kFirst); + ICEBERG_UNWRAP_OR_FAIL(auto sort_order_unique, + SortOrder::Make(*schema, 1, std::vector{sort_field})); + auto sort_order = std::shared_ptr(std::move(sort_order_unique)); + builder->AddSortOrder(sort_order); + + table::SetDefaultSortOrder set_default_update(1); + set_default_update.ApplyTo(*builder); - // Existing table with empty UUID - no requirements - base->table_uuid = ""; - auto empty_uuid_reqs = GenerateRequirements(update, base.get()); - EXPECT_TRUE(empty_uuid_reqs.empty()); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); + EXPECT_EQ(metadata->default_sort_order_id, 1); +} + +TEST(TableUpdateTest, SetDefaultSortOrderGenerateRequirements) { + table::SetDefaultSortOrder update(1); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - generates AssertDefaultSortOrderID requirement + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + ASSERT_EQ(existing_table_reqs.size(), 1); + + // Verify it's the correct requirement type + auto* assert_sort_order = + dynamic_cast(existing_table_reqs[0].get()); + ASSERT_NE(assert_sort_order, nullptr); + EXPECT_EQ(assert_sort_order->sort_order_id(), base->default_sort_order_id); +} + +// Test AddSnapshot +TEST(TableUpdateTest, AddSnapshotGenerateRequirements) { + auto snapshot = std::make_shared(); + table::AddSnapshot update(snapshot); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - no requirements + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + EXPECT_TRUE(existing_table_reqs.empty()); +} + +// Test RemoveSnapshotRef +TEST(TableUpdateTest, RemoveSnapshotRefGenerateRequirements) { + table::RemoveSnapshotRef update("my-branch"); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - no requirements + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + EXPECT_TRUE(existing_table_reqs.empty()); +} + +// Test SetProperties +TEST(TableUpdateTest, SetPropertiesGenerateRequirements) { + table::SetProperties update({{"key", "value"}}); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - no requirements + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + EXPECT_TRUE(existing_table_reqs.empty()); +} + +// Test RemoveProperties +TEST(TableUpdateTest, RemovePropertiesGenerateRequirements) { + table::RemoveProperties update({"key"}); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - no requirements + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + EXPECT_TRUE(existing_table_reqs.empty()); +} + +// Test SetLocation +TEST(TableUpdateTest, SetLocationGenerateRequirements) { + table::SetLocation update("s3://new/location"); + + // New table - no requirements + auto new_table_reqs = GenerateRequirements(update, nullptr); + EXPECT_TRUE(new_table_reqs.empty()); + + // Existing table - no requirements + auto base = CreateBaseMetadata(); + auto existing_table_reqs = GenerateRequirements(update, base.get()); + EXPECT_TRUE(existing_table_reqs.empty()); } } // namespace iceberg diff --git a/src/iceberg/transaction.h b/src/iceberg/transaction.h index 0bcedd6d8..72ba5182c 100644 --- a/src/iceberg/transaction.h +++ b/src/iceberg/transaction.h @@ -21,7 +21,6 @@ #pragma once #include -#include #include "iceberg/iceberg_export.h" #include "iceberg/result.h" From 2df12039f582b42d7c340cf22caef8f945ab6061 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Wed, 3 Dec 2025 15:45:09 +0800 Subject: [PATCH 2/6] 1.1 --- src/iceberg/table_update.h | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/src/iceberg/table_update.h b/src/iceberg/table_update.h index 445295a4d..93b48cf27 100644 --- a/src/iceberg/table_update.h +++ b/src/iceberg/table_update.h @@ -58,8 +58,7 @@ class ICEBERG_EXPORT TableUpdate { /// provides information about the base metadata and operation mode. /// /// \param context The context containing base metadata and operation state - /// \return Status indicating success or failure with error details - virtual Status GenerateRequirements(TableUpdateContext& context) const = 0; + virtual void GenerateRequirements(TableUpdateContext& context) const = 0; }; namespace table { @@ -73,7 +72,7 @@ class ICEBERG_EXPORT AssignUUID : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::string uuid_; @@ -89,7 +88,7 @@ class ICEBERG_EXPORT UpgradeFormatVersion : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: int8_t format_version_; @@ -107,7 +106,7 @@ class ICEBERG_EXPORT AddSchema : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::shared_ptr schema_; @@ -123,7 +122,7 @@ class ICEBERG_EXPORT SetCurrentSchema : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: int32_t schema_id_; @@ -139,7 +138,7 @@ class ICEBERG_EXPORT AddPartitionSpec : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::shared_ptr spec_; @@ -154,7 +153,7 @@ class ICEBERG_EXPORT SetDefaultPartitionSpec : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: int32_t spec_id_; @@ -170,7 +169,7 @@ class ICEBERG_EXPORT RemovePartitionSpecs : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::vector spec_ids_; @@ -186,7 +185,7 @@ class ICEBERG_EXPORT RemoveSchemas : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::vector schema_ids_; @@ -202,7 +201,7 @@ class ICEBERG_EXPORT AddSortOrder : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::shared_ptr sort_order_; @@ -217,7 +216,7 @@ class ICEBERG_EXPORT SetDefaultSortOrder : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: int32_t sort_order_id_; @@ -233,7 +232,7 @@ class ICEBERG_EXPORT AddSnapshot : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::shared_ptr snapshot_; @@ -249,7 +248,7 @@ class ICEBERG_EXPORT RemoveSnapshots : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::vector snapshot_ids_; @@ -264,7 +263,7 @@ class ICEBERG_EXPORT RemoveSnapshotRef : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::string ref_name_; @@ -297,7 +296,7 @@ class ICEBERG_EXPORT SetSnapshotRef : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::string ref_name_; @@ -318,7 +317,7 @@ class ICEBERG_EXPORT SetProperties : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::unordered_map updated_; @@ -334,7 +333,7 @@ class ICEBERG_EXPORT RemoveProperties : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::vector removed_; @@ -349,7 +348,7 @@ class ICEBERG_EXPORT SetLocation : public TableUpdate { void ApplyTo(TableMetadataBuilder& builder) const override; - Status GenerateRequirements(TableUpdateContext& context) const override; + void GenerateRequirements(TableUpdateContext& context) const override; private: std::string location_; From a42319d028054668ee74f06f6d08ba67f849da86 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Wed, 3 Dec 2025 17:47:37 +0800 Subject: [PATCH 3/6] 3 --- src/iceberg/table_requirements.cc | 1 - src/iceberg/test/table_requirements_test.cc | 859 +++++++++++++++++++- src/iceberg/test/table_update_test.cc | 38 - 3 files changed, 857 insertions(+), 41 deletions(-) diff --git a/src/iceberg/table_requirements.cc b/src/iceberg/table_requirements.cc index d0bc86666..753e9a350 100644 --- a/src/iceberg/table_requirements.cc +++ b/src/iceberg/table_requirements.cc @@ -25,7 +25,6 @@ #include "iceberg/table_metadata.h" #include "iceberg/table_requirement.h" #include "iceberg/table_update.h" -#include "iceberg/util/macros.h" namespace iceberg { diff --git a/src/iceberg/test/table_requirements_test.cc b/src/iceberg/test/table_requirements_test.cc index 441e72d41..53c33e081 100644 --- a/src/iceberg/test/table_requirements_test.cc +++ b/src/iceberg/test/table_requirements_test.cc @@ -26,6 +26,7 @@ #include #include "iceberg/partition_spec.h" +#include "iceberg/schema.h" #include "iceberg/snapshot.h" #include "iceberg/sort_order.h" #include "iceberg/table_metadata.h" @@ -47,16 +48,47 @@ std::unique_ptr CreateBaseMetadata( metadata->last_sequence_number = 0; metadata->last_updated_ms = TimePointMs{std::chrono::milliseconds(1000)}; metadata->last_column_id = 0; + metadata->current_schema_id = Schema::kInitialSchemaId; metadata->default_spec_id = PartitionSpec::kInitialSpecId; metadata->last_partition_id = 0; metadata->current_snapshot_id = Snapshot::kInvalidSnapshotId; - metadata->default_sort_order_id = SortOrder::kInitialSortOrderId; + metadata->default_sort_order_id = SortOrder::kUnsortedOrderId; metadata->next_row_id = TableMetadata::kInitialRowId; return metadata; } +// Helper function to create a simple schema for tests +std::shared_ptr CreateTestSchema(int32_t schema_id = 0) { + std::vector fields; + fields.emplace_back(SchemaField::MakeRequired(1, "id", std::make_shared())); + return std::make_shared(std::move(fields), schema_id); +} + +// Helper function to count requirements of a specific type +template +int CountRequirementsOfType( + const std::vector>& requirements) { + int count = 0; + for (const auto& req : requirements) { + if (dynamic_cast(req.get()) != nullptr) { + count++; + } + } + return count; +} + +// Helper function to add a branch to metadata +void AddBranch(TableMetadata& metadata, const std::string& name, int64_t snapshot_id) { + auto ref = std::make_shared(); + ref->snapshot_id = snapshot_id; + ref->retention = SnapshotRef::Branch{}; + metadata.refs[name] = ref; +} + } // namespace +// Empty Updates Tests + TEST(TableRequirementsTest, EmptyUpdatesForCreateTable) { std::vector> updates; @@ -104,6 +136,8 @@ TEST(TableRequirementsTest, EmptyUpdatesForReplaceTable) { EXPECT_EQ(assert_uuid->uuid(), metadata->table_uuid); } +// Table Existence Tests + TEST(TableRequirementsTest, TableAlreadyExists) { std::vector> updates; @@ -134,7 +168,8 @@ TEST(TableRequirementsTest, TableDoesNotExist) { EXPECT_THAT(status, IsOk()); } -// Test for AssignUUID update +// AssignUUID Tests + TEST(TableRequirementsTest, AssignUUID) { auto metadata = CreateBaseMetadata("original-uuid"); std::vector> updates; @@ -207,4 +242,824 @@ TEST(TableRequirementsTest, AssignUUIDForReplaceTable) { EXPECT_THAT(status, IsOk()); } +// UpgradeFormatVersion Tests + +TEST(TableRequirementsTest, UpgradeFormatVersion) { + auto metadata = CreateBaseMetadata(); + std::vector> updates; + updates.push_back(std::make_unique(2)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // UpgradeFormatVersion doesn't add additional requirements + ASSERT_EQ(requirements.size(), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +// AddSchema Tests + +TEST(TableRequirementsTest, AddSchema) { + auto metadata = CreateBaseMetadata(); + metadata->last_column_id = 1; + std::vector> updates; + + auto schema = CreateTestSchema(); + // Add multiple AddSchema updates + updates.push_back(std::make_unique(schema, 1)); + updates.push_back(std::make_unique(schema, 1)); + updates.push_back(std::make_unique(schema, 1)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Should have AssertUUID + AssertLastAssignedFieldId (deduplicated) + ASSERT_EQ(requirements.size(), 2); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Verify the last assigned field ID value + auto* assert_field_id = + dynamic_cast(requirements[1].get()); + ASSERT_NE(assert_field_id, nullptr); + EXPECT_EQ(assert_field_id->last_assigned_field_id(), 1); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +TEST(TableRequirementsTest, AddSchemaFailure) { + auto metadata = CreateBaseMetadata(); + metadata->last_column_id = 2; + + std::vector> updates; + auto schema = CreateTestSchema(); + updates.push_back(std::make_unique(schema, 2)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + + // Create updated metadata with different last_column_id + auto updated = CreateBaseMetadata(); + updated->last_column_id = 3; + + // Find and validate the AssertLastAssignedFieldId requirement + for (const auto& req : requirements) { + if (dynamic_cast(req.get()) != nullptr) { + auto status = req->Validate(updated.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("last assigned field ID does not match")); + break; + } + } +} + +// SetCurrentSchema Tests + +TEST(TableRequirementsTest, SetCurrentSchema) { + auto metadata = CreateBaseMetadata(); + metadata->current_schema_id = 3; + std::vector> updates; + + // Add multiple SetCurrentSchema updates + updates.push_back(std::make_unique(3)); + updates.push_back(std::make_unique(4)); + updates.push_back(std::make_unique(5)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Should have AssertUUID + AssertCurrentSchemaID (deduplicated) + ASSERT_EQ(requirements.size(), 2); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Verify the current schema ID value + auto* assert_schema_id = + dynamic_cast(requirements[1].get()); + ASSERT_NE(assert_schema_id, nullptr); + EXPECT_EQ(assert_schema_id->schema_id(), 3); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +TEST(TableRequirementsTest, SetCurrentSchemaFailure) { + auto metadata = CreateBaseMetadata(); + metadata->current_schema_id = 3; + + std::vector> updates; + updates.push_back(std::make_unique(3)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + + // Create updated metadata with different current_schema_id + auto updated = CreateBaseMetadata(); + updated->current_schema_id = 4; + + // Find and validate the AssertCurrentSchemaID requirement + for (const auto& req : requirements) { + if (dynamic_cast(req.get()) != nullptr) { + auto status = req->Validate(updated.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("current schema ID does not match")); + break; + } + } +} + +// AddPartitionSpec Tests + +TEST(TableRequirementsTest, AddPartitionSpec) { + auto metadata = CreateBaseMetadata(); + metadata->last_partition_id = 3; + + std::vector> updates; + updates.push_back( + std::make_unique(PartitionSpec::Unpartitioned())); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Should have AssertUUID + AssertLastAssignedPartitionId + ASSERT_EQ(requirements.size(), 2); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), + 1); + + // Verify the last assigned partition ID value + auto* assert_partition_id = + dynamic_cast(requirements[1].get()); + ASSERT_NE(assert_partition_id, nullptr); + EXPECT_EQ(assert_partition_id->last_assigned_partition_id(), 3); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +TEST(TableRequirementsTest, AddPartitionSpecFailure) { + auto metadata = CreateBaseMetadata(); + metadata->last_partition_id = 3; + + std::vector> updates; + updates.push_back( + std::make_unique(PartitionSpec::Unpartitioned())); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + + // Create updated metadata with different last_partition_id + auto updated = CreateBaseMetadata(); + updated->last_partition_id = 4; + + // Find and validate the AssertLastAssignedPartitionId requirement + for (const auto& req : requirements) { + if (dynamic_cast(req.get()) != nullptr) { + auto status = req->Validate(updated.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("last assigned partition ID does not match")); + break; + } + } +} + +// SetDefaultPartitionSpec Tests + +TEST(TableRequirementsTest, SetDefaultPartitionSpec) { + auto metadata = CreateBaseMetadata(); + metadata->default_spec_id = 3; + + std::vector> updates; + // Add multiple SetDefaultPartitionSpec updates + updates.push_back(std::make_unique(3)); + updates.push_back(std::make_unique(4)); + updates.push_back(std::make_unique(5)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Should have AssertUUID + AssertDefaultSpecID (deduplicated) + ASSERT_EQ(requirements.size(), 2); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Verify the default spec ID value + auto* assert_spec_id = dynamic_cast(requirements[1].get()); + ASSERT_NE(assert_spec_id, nullptr); + EXPECT_EQ(assert_spec_id->spec_id(), 3); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +TEST(TableRequirementsTest, SetDefaultPartitionSpecFailure) { + auto metadata = CreateBaseMetadata(); + metadata->default_spec_id = PartitionSpec::kInitialSpecId; + + std::vector> updates; + updates.push_back( + std::make_unique(PartitionSpec::kInitialSpecId)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + + // Create updated metadata with different default_spec_id + auto updated = CreateBaseMetadata(); + updated->default_spec_id = PartitionSpec::kInitialSpecId + 1; + + // Find and validate the AssertDefaultSpecID requirement + for (const auto& req : requirements) { + if (dynamic_cast(req.get()) != nullptr) { + auto status = req->Validate(updated.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("default partition spec changed")); + break; + } + } +} + +// RemovePartitionSpecs Tests + +TEST(TableRequirementsTest, RemovePartitionSpecs) { + auto metadata = CreateBaseMetadata(); + metadata->default_spec_id = 3; + + std::vector> updates; + updates.push_back( + std::make_unique(std::vector{1, 2})); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Should have AssertUUID + AssertDefaultSpecID + ASSERT_EQ(requirements.size(), 2); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Verify the default spec ID value + auto* assert_spec_id = dynamic_cast(requirements[1].get()); + ASSERT_NE(assert_spec_id, nullptr); + EXPECT_EQ(assert_spec_id->spec_id(), 3); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +TEST(TableRequirementsTest, RemovePartitionSpecsWithBranch) { + auto metadata = CreateBaseMetadata(); + metadata->default_spec_id = 3; + AddBranch(*metadata, "branch", 42); + + std::vector> updates; + updates.push_back( + std::make_unique(std::vector{1, 2})); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Should have AssertUUID + AssertDefaultSpecID + AssertRefSnapshotID + ASSERT_EQ(requirements.size(), 3); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +TEST(TableRequirementsTest, RemovePartitionSpecsWithSpecChangedFailure) { + auto metadata = CreateBaseMetadata(); + metadata->default_spec_id = 3; + + std::vector> updates; + updates.push_back( + std::make_unique(std::vector{1, 2})); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + + // Create updated metadata with different default_spec_id + auto updated = CreateBaseMetadata(); + updated->default_spec_id = 4; + + // Find and validate the AssertDefaultSpecID requirement + for (const auto& req : requirements) { + if (dynamic_cast(req.get()) != nullptr) { + auto status = req->Validate(updated.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("default partition spec changed")); + break; + } + } +} + +TEST(TableRequirementsTest, RemovePartitionSpecsWithBranchChangedFailure) { + auto metadata = CreateBaseMetadata(); + metadata->default_spec_id = 3; + AddBranch(*metadata, "test", 42); + + std::vector> updates; + updates.push_back( + std::make_unique(std::vector{1, 2})); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + + // Create updated metadata with changed branch + auto updated = CreateBaseMetadata(); + updated->default_spec_id = 3; + AddBranch(*updated, "test", 43); + + // Find and validate the AssertRefSnapshotID requirement + for (const auto& req : requirements) { + if (dynamic_cast(req.get()) != nullptr) { + auto status = req->Validate(updated.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("has changed")); + break; + } + } +} + +// RemoveSchemas Tests + +TEST(TableRequirementsTest, RemoveSchemas) { + auto metadata = CreateBaseMetadata(); + metadata->current_schema_id = 3; + + std::vector> updates; + updates.push_back(std::make_unique(std::vector{1, 2})); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Should have AssertUUID + AssertCurrentSchemaID + ASSERT_EQ(requirements.size(), 2); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Verify the current schema ID value + auto* assert_schema_id = + dynamic_cast(requirements[1].get()); + ASSERT_NE(assert_schema_id, nullptr); + EXPECT_EQ(assert_schema_id->schema_id(), 3); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +TEST(TableRequirementsTest, RemoveSchemasWithBranch) { + auto metadata = CreateBaseMetadata(); + metadata->current_schema_id = 3; + AddBranch(*metadata, "branch", 42); + + std::vector> updates; + updates.push_back(std::make_unique(std::vector{1, 2})); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Should have AssertUUID + AssertCurrentSchemaID + AssertRefSnapshotID + ASSERT_EQ(requirements.size(), 3); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +TEST(TableRequirementsTest, RemoveSchemasWithSchemaChangedFailure) { + auto metadata = CreateBaseMetadata(); + metadata->current_schema_id = 3; + + std::vector> updates; + updates.push_back(std::make_unique(std::vector{1, 2})); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + + // Create updated metadata with different current_schema_id + auto updated = CreateBaseMetadata(); + updated->current_schema_id = 4; + + // Find and validate the AssertCurrentSchemaID requirement + for (const auto& req : requirements) { + if (dynamic_cast(req.get()) != nullptr) { + auto status = req->Validate(updated.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("current schema ID does not match")); + break; + } + } +} + +TEST(TableRequirementsTest, RemoveSchemasWithBranchChangedFailure) { + auto metadata = CreateBaseMetadata(); + metadata->current_schema_id = 3; + AddBranch(*metadata, "test", 42); + + std::vector> updates; + updates.push_back(std::make_unique(std::vector{1, 2})); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + + // Create updated metadata with changed branch + auto updated = CreateBaseMetadata(); + updated->current_schema_id = 3; + AddBranch(*updated, "test", 43); + + // Find and validate the AssertRefSnapshotID requirement + for (const auto& req : requirements) { + if (dynamic_cast(req.get()) != nullptr) { + auto status = req->Validate(updated.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("has changed")); + break; + } + } +} + +// AddSortOrder Tests + +TEST(TableRequirementsTest, AddSortOrder) { + auto metadata = CreateBaseMetadata(); + std::vector> updates; + + updates.push_back(std::make_unique(SortOrder::Unsorted())); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // AddSortOrder doesn't add additional requirements + ASSERT_EQ(requirements.size(), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +// SetDefaultSortOrder Tests + +TEST(TableRequirementsTest, SetDefaultSortOrder) { + auto metadata = CreateBaseMetadata(); + metadata->default_sort_order_id = 3; + + std::vector> updates; + // Add multiple SetDefaultSortOrder updates + updates.push_back(std::make_unique(3)); + updates.push_back(std::make_unique(4)); + updates.push_back(std::make_unique(5)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Should have AssertUUID + AssertDefaultSortOrderID (deduplicated) + ASSERT_EQ(requirements.size(), 2); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Verify the default sort order ID value + auto* assert_sort_order_id = + dynamic_cast(requirements[1].get()); + ASSERT_NE(assert_sort_order_id, nullptr); + EXPECT_EQ(assert_sort_order_id->sort_order_id(), 3); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +TEST(TableRequirementsTest, SetDefaultSortOrderFailure) { + auto metadata = CreateBaseMetadata(); + metadata->default_sort_order_id = SortOrder::kUnsortedOrderId; + + std::vector> updates; + updates.push_back( + std::make_unique(SortOrder::kUnsortedOrderId)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + + // Create updated metadata with different default_sort_order_id + auto updated = CreateBaseMetadata(); + updated->default_sort_order_id = SortOrder::kUnsortedOrderId + 1; + + // Find and validate the AssertDefaultSortOrderID requirement + for (const auto& req : requirements) { + if (dynamic_cast(req.get()) != nullptr) { + auto status = req->Validate(updated.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("default sort order changed")); + break; + } + } +} + +// AddSnapshot Tests + +TEST(TableRequirementsTest, AddSnapshot) { + auto metadata = CreateBaseMetadata(); + std::vector> updates; + + auto snapshot = std::make_shared(); + snapshot->snapshot_id = 1; + snapshot->sequence_number = 1; + snapshot->timestamp_ms = TimePointMs{std::chrono::milliseconds(1000)}; + snapshot->manifest_list = "s3://bucket/manifest_list"; + + updates.push_back(std::make_unique(snapshot)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // AddSnapshot doesn't add additional requirements + ASSERT_EQ(requirements.size(), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +// RemoveSnapshotRef Tests + +TEST(TableRequirementsTest, RemoveSnapshotRef) { + auto metadata = CreateBaseMetadata(); + std::vector> updates; + + updates.push_back(std::make_unique("branch")); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // RemoveSnapshotRef doesn't add additional requirements + ASSERT_EQ(requirements.size(), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +// SetAndRemoveProperties Tests + +TEST(TableRequirementsTest, SetProperties) { + auto metadata = CreateBaseMetadata(); + std::vector> updates; + + std::unordered_map props; + props["test"] = "value"; + updates.push_back(std::make_unique(props)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // SetProperties doesn't add additional requirements + ASSERT_EQ(requirements.size(), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +TEST(TableRequirementsTest, RemoveProperties) { + auto metadata = CreateBaseMetadata(); + std::vector> updates; + + updates.push_back( + std::make_unique(std::vector{"test"})); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // RemoveProperties doesn't add additional requirements + ASSERT_EQ(requirements.size(), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +// SetLocation Tests + +TEST(TableRequirementsTest, SetLocation) { + auto metadata = CreateBaseMetadata(); + std::vector> updates; + + updates.push_back(std::make_unique("s3://new-bucket/test")); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // SetLocation doesn't add additional requirements + ASSERT_EQ(requirements.size(), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + // Validate against base metadata + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +// AssertRefSnapshotID Tests + +TEST(TableRequirementsTest, AssertRefSnapshotIDSuccess) { + auto metadata = CreateBaseMetadata(); + AddBranch(*metadata, "branch", 14); + + table::AssertRefSnapshotID requirement("branch", 14); + auto status = requirement.Validate(metadata.get()); + EXPECT_THAT(status, IsOk()); +} + +TEST(TableRequirementsTest, AssertRefSnapshotIDCreatedConcurrently) { + auto metadata = CreateBaseMetadata(); + AddBranch(*metadata, "random_branch", 14); + + // Requirement expects ref doesn't exist (nullopt snapshot_id) + table::AssertRefSnapshotID requirement("random_branch", std::nullopt); + auto status = requirement.Validate(metadata.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("was created concurrently")); +} + +TEST(TableRequirementsTest, AssertRefSnapshotIDMissing) { + auto metadata = CreateBaseMetadata(); + // No branch added + + // Requirement expects a snapshot ID that doesn't exist + table::AssertRefSnapshotID requirement("random_branch", 14); + auto status = requirement.Validate(metadata.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("is missing")); +} + +TEST(TableRequirementsTest, AssertRefSnapshotIDChanged) { + auto metadata = CreateBaseMetadata(); + AddBranch(*metadata, "random_branch", 15); + + // Requirement expects snapshot ID 14, but actual is 15 + table::AssertRefSnapshotID requirement("random_branch", 14); + auto status = requirement.Validate(metadata.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("has changed")); +} + +// Replace Table Tests (less restrictive than Update) + +TEST(TableRequirementsTest, ReplaceTableDoesNotRequireCurrentSchemaID) { + auto metadata = CreateBaseMetadata(); + metadata->current_schema_id = 3; + + std::vector> updates; + updates.push_back(std::make_unique(5)); + + auto result = TableRequirements::ForReplaceTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Replace table should NOT add AssertCurrentSchemaID + EXPECT_EQ(CountRequirementsOfType(requirements), 0); +} + +TEST(TableRequirementsTest, ReplaceTableDoesNotRequireDefaultSpecID) { + auto metadata = CreateBaseMetadata(); + metadata->default_spec_id = 3; + + std::vector> updates; + updates.push_back(std::make_unique(5)); + + auto result = TableRequirements::ForReplaceTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Replace table should NOT add AssertDefaultSpecID + EXPECT_EQ(CountRequirementsOfType(requirements), 0); +} + +TEST(TableRequirementsTest, ReplaceTableDoesNotRequireDefaultSortOrderID) { + auto metadata = CreateBaseMetadata(); + metadata->default_sort_order_id = 3; + + std::vector> updates; + updates.push_back(std::make_unique(5)); + + auto result = TableRequirements::ForReplaceTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Replace table should NOT add AssertDefaultSortOrderID + EXPECT_EQ(CountRequirementsOfType(requirements), 0); +} + +TEST(TableRequirementsTest, ReplaceTableDoesNotAddBranchRequirements) { + auto metadata = CreateBaseMetadata(); + metadata->current_schema_id = 3; + AddBranch(*metadata, "branch", 42); + + std::vector> updates; + updates.push_back(std::make_unique(std::vector{1, 2})); + + auto result = TableRequirements::ForReplaceTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Replace table should NOT add AssertRefSnapshotID for branches + EXPECT_EQ(CountRequirementsOfType(requirements), 0); +} + +// Combined Updates Tests + +TEST(TableRequirementsTest, MultipleUpdatesDeduplication) { + auto metadata = CreateBaseMetadata(); + metadata->last_column_id = 1; + metadata->current_schema_id = 0; + + std::vector> updates; + auto schema = CreateTestSchema(); + // Add multiple AddSchema updates - should only generate one requirement + updates.push_back(std::make_unique(schema, 1)); + updates.push_back(std::make_unique(schema, 1)); + // Add multiple SetCurrentSchema updates - should only generate one requirement + updates.push_back(std::make_unique(0)); + updates.push_back(std::make_unique(1)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Should have: 1 AssertUUID + 1 AssertLastAssignedFieldId + 1 AssertCurrentSchemaID + ASSERT_EQ(requirements.size(), 3); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); +} + } // namespace iceberg diff --git a/src/iceberg/test/table_update_test.cc b/src/iceberg/test/table_update_test.cc index e7d725d57..6da950d31 100644 --- a/src/iceberg/test/table_update_test.cc +++ b/src/iceberg/test/table_update_test.cc @@ -293,25 +293,6 @@ TEST(TableUpdateTest, RemoveSchemasGenerateRequirements) { } // Test AddSortOrder -TEST(TableUpdateTest, ApplyAddSortOrderUpdate) { - auto base = CreateBaseMetadata(); - auto builder = TableMetadataBuilder::BuildFrom(base.get()); - - // Apply AddSortOrder update - auto schema = CreateTestSchema(); - SortField sort_field(1, Transform::Identity(), SortDirection::kAscending, - NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL(auto sort_order_unique, - SortOrder::Make(*schema, 1, std::vector{sort_field})); - auto sort_order = std::shared_ptr(std::move(sort_order_unique)); - table::AddSortOrder sort_order_update(sort_order); - sort_order_update.ApplyTo(*builder); - - ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); - ASSERT_EQ(metadata->sort_orders.size(), 2); - EXPECT_EQ(metadata->sort_orders[1]->order_id(), 1); -} - TEST(TableUpdateTest, AddSortOrderGenerateRequirements) { auto schema = CreateTestSchema(); SortField sort_field(1, Transform::Identity(), SortDirection::kAscending, @@ -331,25 +312,6 @@ TEST(TableUpdateTest, AddSortOrderGenerateRequirements) { } // Test SetDefaultSortOrder -TEST(TableUpdateTest, ApplySetDefaultSortOrderUpdate) { - auto base = CreateBaseMetadata(); - auto builder = TableMetadataBuilder::BuildFrom(base.get()); - - auto schema = CreateTestSchema(); - SortField sort_field(1, Transform::Identity(), SortDirection::kAscending, - NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL(auto sort_order_unique, - SortOrder::Make(*schema, 1, std::vector{sort_field})); - auto sort_order = std::shared_ptr(std::move(sort_order_unique)); - builder->AddSortOrder(sort_order); - - table::SetDefaultSortOrder set_default_update(1); - set_default_update.ApplyTo(*builder); - - ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); - EXPECT_EQ(metadata->default_sort_order_id, 1); -} - TEST(TableUpdateTest, SetDefaultSortOrderGenerateRequirements) { table::SetDefaultSortOrder update(1); From 6541d5e63039166c5f50a34016c6fffc152fdcc4 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Wed, 3 Dec 2025 19:15:59 +0800 Subject: [PATCH 4/6] use parameter struct for testing GenerateRequirements behavior --- src/iceberg/test/table_update_test.cc | 530 +++++++++++--------------- 1 file changed, 221 insertions(+), 309 deletions(-) diff --git a/src/iceberg/test/table_update_test.cc b/src/iceberg/test/table_update_test.cc index 6da950d31..d250b6d20 100644 --- a/src/iceberg/test/table_update_test.cc +++ b/src/iceberg/test/table_update_test.cc @@ -19,6 +19,7 @@ #include "iceberg/table_update.h" +#include #include #include #include @@ -81,325 +82,236 @@ std::unique_ptr CreateBaseMetadata() { } // namespace -// Test AssignUUID -TEST(TableUpdateTest, AssignUUIDApplyUpdate) { - auto base = CreateBaseMetadata(); - auto builder = TableMetadataBuilder::BuildFrom(base.get()); - - // Apply AssignUUID update - table::AssignUUID uuid_update("apply-uuid"); - uuid_update.ApplyTo(*builder); - - ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); - EXPECT_EQ(metadata->table_uuid, "apply-uuid"); -} - -TEST(TableUpdateTest, AssignUUIDGenerateRequirements) { - table::AssignUUID update("new-uuid"); - - // New table - no requirements (AssignUUID doesn't generate requirements) - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table - no requirements - auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - EXPECT_TRUE(existing_table_reqs.empty()); -} - -// Test UpgradeFormatVersion -TEST(TableUpdateTest, UpgradeFormatVersionGenerateRequirements) { - table::UpgradeFormatVersion update(3); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table - no requirements - auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - EXPECT_TRUE(existing_table_reqs.empty()); -} - -// Test AddSchema -TEST(TableUpdateTest, AddSchemaGenerateRequirements) { - auto new_schema = std::make_shared( - std::vector{SchemaField::MakeRequired(4, "new_col", string())}, 3); - table::AddSchema update(new_schema, 3); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); +// Parameter struct for testing GenerateRequirements behavior +struct GenerateRequirementsTestParam { + std::string test_name; + std::function()> update_factory; + // Expected number of requirements for existing table (new table always expects 0) + size_t expected_existing_table_count; + // Optional validator function to check the generated requirements + std::function>&, + const TableMetadata*)> + validator; +}; + +class GenerateRequirementsTest + : public ::testing::TestWithParam {}; + +TEST_P(GenerateRequirementsTest, GeneratesExpectedRequirements) { + const auto& param = GetParam(); + auto update = param.update_factory(); + + // New table - always no requirements + auto new_table_reqs = GenerateRequirements(*update, nullptr); EXPECT_TRUE(new_table_reqs.empty()); - // Existing table - generates AssertLastAssignedFieldId requirement + // Existing table - check expected count auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - ASSERT_EQ(existing_table_reqs.size(), 1); - - auto* assert_id = - dynamic_cast(existing_table_reqs[0].get()); - ASSERT_NE(assert_id, nullptr); - EXPECT_EQ(assert_id->last_assigned_field_id(), base->last_column_id); -} - -// Test SetCurrentSchema -TEST(TableUpdateTest, SetCurrentSchemaGenerateRequirements) { - table::SetCurrentSchema update(1); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table - generates AssertCurrentSchemaID requirement - auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - ASSERT_EQ(existing_table_reqs.size(), 1); - - auto* assert_id = - dynamic_cast(existing_table_reqs[0].get()); - ASSERT_NE(assert_id, nullptr); - EXPECT_EQ(assert_id->schema_id(), base->current_schema_id); -} - -// Test AddPartitionSpec -TEST(TableUpdateTest, AddPartitionSpecGenerateRequirements) { - PartitionField partition_field(1, 1, "id_identity", Transform::Identity()); - ICEBERG_UNWRAP_OR_FAIL(auto spec_unique, PartitionSpec::Make(1, {partition_field})); - auto spec = std::shared_ptr(std::move(spec_unique)); - table::AddPartitionSpec update(spec); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table - generates AssertLastAssignedPartitionId requirement - auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - ASSERT_EQ(existing_table_reqs.size(), 1); - - auto* assert_id = - dynamic_cast(existing_table_reqs[0].get()); - ASSERT_NE(assert_id, nullptr); - EXPECT_EQ(assert_id->last_assigned_partition_id(), base->last_partition_id); -} - -// Test SetDefaultPartitionSpec -TEST(TableUpdateTest, SetDefaultPartitionSpecGenerateRequirements) { - table::SetDefaultPartitionSpec update(1); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table - generates AssertDefaultSpecID requirement - auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - ASSERT_EQ(existing_table_reqs.size(), 1); - - auto* assert_id = - dynamic_cast(existing_table_reqs[0].get()); - ASSERT_NE(assert_id, nullptr); - EXPECT_EQ(assert_id->spec_id(), base->default_spec_id); -} - -// Test RemovePartitionSpecs -TEST(TableUpdateTest, RemovePartitionSpecsGenerateRequirements) { - table::RemovePartitionSpecs update(std::vector{1}); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table without snapshot - one requirement - auto base = CreateBaseMetadata(); - auto existing_table_reqs_no_snapshot = GenerateRequirements(update, base.get()); - ASSERT_EQ(existing_table_reqs_no_snapshot.size(), 1); - auto* assert_spec_id = - dynamic_cast(existing_table_reqs_no_snapshot[0].get()); - ASSERT_NE(assert_spec_id, nullptr); - EXPECT_EQ(assert_spec_id->spec_id(), base->default_spec_id); - - // Existing table with a non-main branch - two requirements - base->current_snapshot_id = 12345; - auto snapshot = std::make_shared(); - snapshot->snapshot_id = 12345; - snapshot->sequence_number = 1; - base->snapshots.push_back(snapshot); - const std::string dev_branch = "dev"; - base->refs.emplace(dev_branch, - std::make_shared(SnapshotRef{ - .snapshot_id = 12345, .retention = SnapshotRef::Branch{}})); - - auto existing_table_reqs_with_snapshot = GenerateRequirements(update, base.get()); - ASSERT_EQ(existing_table_reqs_with_snapshot.size(), 2); + auto existing_table_reqs = GenerateRequirements(*update, base.get()); + ASSERT_EQ(existing_table_reqs.size(), param.expected_existing_table_count); - auto* req1 = - dynamic_cast(existing_table_reqs_no_snapshot[0].get()); - EXPECT_EQ(req1->spec_id(), base->default_spec_id); - auto* req2 = dynamic_cast( - existing_table_reqs_with_snapshot[1].get()); - EXPECT_EQ(req2->ref_name(), dev_branch); - EXPECT_TRUE(req2->snapshot_id().has_value()); - EXPECT_EQ(req2->snapshot_id().value(), base->current_snapshot_id); -} - -// Test RemoveSchemas -TEST(TableUpdateTest, RemoveSchemasGenerateRequirements) { - table::RemoveSchemas update({1}); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table without snapshot - one requirement - auto base = CreateBaseMetadata(); - auto existing_table_reqs_no_snapshot = GenerateRequirements(update, base.get()); - ASSERT_EQ(existing_table_reqs_no_snapshot.size(), 1); - auto* assert_schema_id = dynamic_cast( - existing_table_reqs_no_snapshot[0].get()); - ASSERT_NE(assert_schema_id, nullptr); - EXPECT_EQ(assert_schema_id->schema_id(), base->current_schema_id); - - // Existing table with a non-main branch - two requirements - base->current_snapshot_id = 12345; - auto snapshot = std::make_shared(); - snapshot->snapshot_id = 12345; - snapshot->sequence_number = 1; - base->snapshots.push_back(snapshot); - const std::string dev_branch = "dev"; - base->refs.emplace(dev_branch, - std::make_shared(SnapshotRef{ - .snapshot_id = 12345, .retention = SnapshotRef::Branch{}})); - - auto existing_table_reqs_with_snapshot = GenerateRequirements(update, base.get()); - ASSERT_EQ(existing_table_reqs_with_snapshot.size(), 2); - - // The order is not guaranteed. - bool found_schema_id_req = false; - bool found_branch_req = false; - for (const auto& req : existing_table_reqs_with_snapshot) { - if (auto* r = dynamic_cast(req.get())) { - EXPECT_EQ(r->schema_id(), base->current_schema_id); - found_schema_id_req = true; - } else if (auto* r = dynamic_cast(req.get())) { - EXPECT_EQ(r->ref_name(), dev_branch); - EXPECT_TRUE(r->snapshot_id().has_value()); - EXPECT_EQ(r->snapshot_id().value(), base->current_snapshot_id); - found_branch_req = true; - } + // Validate the requirements if validator is provided + if (param.validator) { + param.validator(existing_table_reqs, base.get()); } - EXPECT_TRUE(found_schema_id_req); - EXPECT_TRUE(found_branch_req); -} - -// Test AddSortOrder -TEST(TableUpdateTest, AddSortOrderGenerateRequirements) { - auto schema = CreateTestSchema(); - SortField sort_field(1, Transform::Identity(), SortDirection::kAscending, - NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL(std::shared_ptr sort_order, - SortOrder::Make(*schema, 1, std::vector{sort_field})); - table::AddSortOrder update(sort_order); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table - no requirements - auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - EXPECT_TRUE(existing_table_reqs.empty()); -} - -// Test SetDefaultSortOrder -TEST(TableUpdateTest, SetDefaultSortOrderGenerateRequirements) { - table::SetDefaultSortOrder update(1); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table - generates AssertDefaultSortOrderID requirement - auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - ASSERT_EQ(existing_table_reqs.size(), 1); - - // Verify it's the correct requirement type - auto* assert_sort_order = - dynamic_cast(existing_table_reqs[0].get()); - ASSERT_NE(assert_sort_order, nullptr); - EXPECT_EQ(assert_sort_order->sort_order_id(), base->default_sort_order_id); -} - -// Test AddSnapshot -TEST(TableUpdateTest, AddSnapshotGenerateRequirements) { - auto snapshot = std::make_shared(); - table::AddSnapshot update(snapshot); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table - no requirements - auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - EXPECT_TRUE(existing_table_reqs.empty()); -} - -// Test RemoveSnapshotRef -TEST(TableUpdateTest, RemoveSnapshotRefGenerateRequirements) { - table::RemoveSnapshotRef update("my-branch"); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table - no requirements - auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - EXPECT_TRUE(existing_table_reqs.empty()); } -// Test SetProperties -TEST(TableUpdateTest, SetPropertiesGenerateRequirements) { - table::SetProperties update({{"key", "value"}}); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table - no requirements - auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - EXPECT_TRUE(existing_table_reqs.empty()); -} - -// Test RemoveProperties -TEST(TableUpdateTest, RemovePropertiesGenerateRequirements) { - table::RemoveProperties update({"key"}); - - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); - - // Existing table - no requirements +INSTANTIATE_TEST_SUITE_P( + TableUpdateGenerateRequirements, GenerateRequirementsTest, + ::testing::Values( + // Updates that generate no requirements + GenerateRequirementsTestParam{ + .test_name = "AssignUUID", + .update_factory = + [] { return std::make_unique("new-uuid"); }, + .expected_existing_table_count = 0, + .validator = nullptr}, + GenerateRequirementsTestParam{ + .test_name = "UpgradeFormatVersion", + .update_factory = + [] { return std::make_unique(3); }, + .expected_existing_table_count = 0, + .validator = nullptr}, + GenerateRequirementsTestParam{ + .test_name = "AddSortOrder", + .update_factory = + [] { + auto schema = CreateTestSchema(); + SortField sort_field(1, Transform::Identity(), + SortDirection::kAscending, NullOrder::kFirst); + auto sort_order = + SortOrder::Make(*schema, 1, std::vector{sort_field}) + .value(); + return std::make_unique(std::move(sort_order)); + }, + .expected_existing_table_count = 0, + .validator = nullptr}, + GenerateRequirementsTestParam{.test_name = "AddSnapshot", + .update_factory = + [] { + auto snapshot = std::make_shared(); + return std::make_unique( + snapshot); + }, + .expected_existing_table_count = 0, + .validator = nullptr}, + GenerateRequirementsTestParam{ + .test_name = "RemoveSnapshotRef", + .update_factory = + [] { return std::make_unique("my-branch"); }, + .expected_existing_table_count = 0, + .validator = nullptr}, + GenerateRequirementsTestParam{ + .test_name = "SetProperties", + .update_factory = + [] { + return std::make_unique( + std::unordered_map{{"key", "value"}}); + }, + .expected_existing_table_count = 0, + .validator = nullptr}, + GenerateRequirementsTestParam{ + .test_name = "RemoveProperties", + .update_factory = + [] { + return std::make_unique( + std::vector{"key"}); + }, + .expected_existing_table_count = 0, + .validator = nullptr}, + GenerateRequirementsTestParam{ + .test_name = "SetLocation", + .update_factory = + [] { return std::make_unique("s3://new/location"); }, + .expected_existing_table_count = 0, + .validator = nullptr}, + + // Updates that generate single requirement for existing tables + GenerateRequirementsTestParam{ + .test_name = "AddSchema", + .update_factory = + [] { + auto new_schema = std::make_shared( + std::vector{ + SchemaField::MakeRequired(4, "new_col", string())}, + 3); + return std::make_unique(new_schema, 3); + }, + .expected_existing_table_count = 1, + .validator = + [](const std::vector>& reqs, + const TableMetadata* base) { + auto* assert_id = dynamic_cast( + reqs[0].get()); + ASSERT_NE(assert_id, nullptr); + EXPECT_EQ(assert_id->last_assigned_field_id(), base->last_column_id); + }}, + GenerateRequirementsTestParam{ + .test_name = "SetCurrentSchema", + .update_factory = [] { return std::make_unique(1); }, + .expected_existing_table_count = 1, + .validator = + [](const std::vector>& reqs, + const TableMetadata* base) { + auto* assert_id = + dynamic_cast(reqs[0].get()); + ASSERT_NE(assert_id, nullptr); + EXPECT_EQ(assert_id->schema_id(), base->current_schema_id); + }}, + GenerateRequirementsTestParam{ + .test_name = "AddPartitionSpec", + .update_factory = + [] { + PartitionField partition_field(1, 1, "id_identity", + Transform::Identity()); + auto spec = std::shared_ptr( + PartitionSpec::Make(1, {partition_field}).value().release()); + return std::make_unique(spec); + }, + .expected_existing_table_count = 1, + .validator = + [](const std::vector>& reqs, + const TableMetadata* base) { + auto* assert_id = + dynamic_cast( + reqs[0].get()); + ASSERT_NE(assert_id, nullptr); + EXPECT_EQ(assert_id->last_assigned_partition_id(), + base->last_partition_id); + }}, + GenerateRequirementsTestParam{ + .test_name = "SetDefaultPartitionSpec", + .update_factory = + [] { return std::make_unique(1); }, + .expected_existing_table_count = 1, + .validator = + [](const std::vector>& reqs, + const TableMetadata* base) { + auto* assert_id = + dynamic_cast(reqs[0].get()); + ASSERT_NE(assert_id, nullptr); + EXPECT_EQ(assert_id->spec_id(), base->default_spec_id); + }}, + GenerateRequirementsTestParam{ + .test_name = "SetDefaultSortOrder", + .update_factory = + [] { return std::make_unique(1); }, + .expected_existing_table_count = 1, + .validator = + [](const std::vector>& reqs, + const TableMetadata* base) { + auto* assert_sort_order = + dynamic_cast(reqs[0].get()); + ASSERT_NE(assert_sort_order, nullptr); + EXPECT_EQ(assert_sort_order->sort_order_id(), + base->default_sort_order_id); + }}, + GenerateRequirementsTestParam{ + .test_name = "RemovePartitionSpecs", + .update_factory = + [] { + return std::make_unique( + std::vector{1}); + }, + .expected_existing_table_count = 1, + .validator = + [](const std::vector>& reqs, + const TableMetadata* base) { + auto* assert_id = + dynamic_cast(reqs[0].get()); + ASSERT_NE(assert_id, nullptr); + EXPECT_EQ(assert_id->spec_id(), base->default_spec_id); + }}, + GenerateRequirementsTestParam{ + .test_name = "RemoveSchemas", + .update_factory = + [] { + return std::make_unique(std::vector{1}); + }, + .expected_existing_table_count = 1, + .validator = + [](const std::vector>& reqs, + const TableMetadata* base) { + auto* assert_id = + dynamic_cast(reqs[0].get()); + ASSERT_NE(assert_id, nullptr); + EXPECT_EQ(assert_id->schema_id(), base->current_schema_id); + }}), + [](const testing::TestParamInfo& info) { + return info.param.test_name; + }); + +// Test AssignUUID ApplyTo +TEST(TableUpdateTest, AssignUUIDApplyUpdate) { auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - EXPECT_TRUE(existing_table_reqs.empty()); -} - -// Test SetLocation -TEST(TableUpdateTest, SetLocationGenerateRequirements) { - table::SetLocation update("s3://new/location"); + auto builder = TableMetadataBuilder::BuildFrom(base.get()); - // New table - no requirements - auto new_table_reqs = GenerateRequirements(update, nullptr); - EXPECT_TRUE(new_table_reqs.empty()); + // Apply AssignUUID update + table::AssignUUID uuid_update("apply-uuid"); + uuid_update.ApplyTo(*builder); - // Existing table - no requirements - auto base = CreateBaseMetadata(); - auto existing_table_reqs = GenerateRequirements(update, base.get()); - EXPECT_TRUE(existing_table_reqs.empty()); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); + EXPECT_EQ(metadata->table_uuid, "apply-uuid"); } } // namespace iceberg From 4044a87abf53e9221552ad35414e6c5ce9d76056 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Thu, 4 Dec 2025 11:54:09 +0800 Subject: [PATCH 5/6] 1 --- src/iceberg/table_requirements.cc | 5 ++ src/iceberg/table_requirements.h | 10 +++ src/iceberg/table_update.cc | 13 +++- src/iceberg/test/table_requirements_test.cc | 79 +++++++++++++++++---- 4 files changed, 90 insertions(+), 17 deletions(-) diff --git a/src/iceberg/table_requirements.cc b/src/iceberg/table_requirements.cc index 753e9a350..99ffcdafe 100644 --- a/src/iceberg/table_requirements.cc +++ b/src/iceberg/table_requirements.cc @@ -97,6 +97,11 @@ void TableUpdateContext::RequireNoBranchesChanged() { } } +bool TableUpdateContext::AddChangedRef(const std::string& ref_name) { + auto [it, inserted] = changed_refs_.insert(ref_name); + return inserted; +} + Result>> TableRequirements::ForCreateTable( const std::vector>& table_updates) { TableUpdateContext context(nullptr, false); diff --git a/src/iceberg/table_requirements.h b/src/iceberg/table_requirements.h index 7e5c0e74f..f79f0bead 100644 --- a/src/iceberg/table_requirements.h +++ b/src/iceberg/table_requirements.h @@ -27,6 +27,8 @@ /// for optimistic concurrency control when committing table changes. #include +#include +#include #include #include "iceberg/iceberg_export.h" @@ -82,6 +84,11 @@ class ICEBERG_EXPORT TableUpdateContext { /// \brief Require that no branches have been changed void RequireNoBranchesChanged(); + /// \brief Track a changed ref and return whether it was newly added + /// \param ref_name The name of the ref being changed + /// \return true if this is the first time the ref is being changed + bool AddChangedRef(const std::string& ref_name); + private: const TableMetadata* base_; const bool is_replace_; @@ -94,6 +101,9 @@ class ICEBERG_EXPORT TableUpdateContext { bool added_last_assigned_partition_id_ = false; bool added_default_spec_id_ = false; bool added_default_sort_order_id_ = false; + + // Track refs that have been changed to avoid duplicate requirements + std::unordered_set changed_refs_; }; /// \brief Factory class for generating table requirements diff --git a/src/iceberg/table_update.cc b/src/iceberg/table_update.cc index 2e1c82783..90f7de622 100644 --- a/src/iceberg/table_update.cc +++ b/src/iceberg/table_update.cc @@ -142,7 +142,7 @@ void AddSnapshot::GenerateRequirements(TableUpdateContext& context) const { void RemoveSnapshots::ApplyTo(TableMetadataBuilder& builder) const {} void RemoveSnapshots::GenerateRequirements(TableUpdateContext& context) const { - throw IcebergError("RemoveTableSnapshots::GenerateRequirements not implemented"); + // RemoveSnapshots doesn't generate any requirements } // RemoveSnapshotRef @@ -162,7 +162,16 @@ void SetSnapshotRef::ApplyTo(TableMetadataBuilder& builder) const { } void SetSnapshotRef::GenerateRequirements(TableUpdateContext& context) const { - throw NotImplemented("SetTableSnapshotRef::GenerateRequirements not implemented"); + bool added = context.AddChangedRef(ref_name_); + if (added && context.base() != nullptr && !context.is_replace()) { + const auto& refs = context.base()->refs; + auto it = refs.find(ref_name_); + // Require that the ref does not exist (nullopt) or is the same as the base snapshot + std::optional base_snapshot_id = + (it != refs.end()) ? std::make_optional(it->second->snapshot_id) : std::nullopt; + context.AddRequirement( + std::make_unique(ref_name_, base_snapshot_id)); + } } // SetProperties diff --git a/src/iceberg/test/table_requirements_test.cc b/src/iceberg/test/table_requirements_test.cc index 53c33e081..041b44dd1 100644 --- a/src/iceberg/test/table_requirements_test.cc +++ b/src/iceberg/test/table_requirements_test.cc @@ -20,6 +20,7 @@ #include "iceberg/table_requirements.h" #include +#include #include #include @@ -33,6 +34,7 @@ #include "iceberg/table_requirement.h" #include "iceberg/table_update.h" #include "iceberg/test/matchers.h" +#include "iceberg/type.h" namespace iceberg { @@ -60,7 +62,7 @@ std::unique_ptr CreateBaseMetadata( // Helper function to create a simple schema for tests std::shared_ptr CreateTestSchema(int32_t schema_id = 0) { std::vector fields; - fields.emplace_back(SchemaField::MakeRequired(1, "id", std::make_shared())); + fields.emplace_back(SchemaField::MakeRequired(1, "id", int32())); return std::make_shared(std::move(fields), schema_id); } @@ -68,13 +70,9 @@ std::shared_ptr CreateTestSchema(int32_t schema_id = 0) { template int CountRequirementsOfType( const std::vector>& requirements) { - int count = 0; - for (const auto& req : requirements) { - if (dynamic_cast(req.get()) != nullptr) { - count++; - } - } - return count; + return std::ranges::count_if(requirements, [](const auto& req) { + return dynamic_cast(req.get()) != nullptr; + }); } // Helper function to add a branch to metadata @@ -815,47 +813,98 @@ TEST(TableRequirementsTest, SetDefaultSortOrderFailure) { TEST(TableRequirementsTest, AddSnapshot) { auto metadata = CreateBaseMetadata(); - std::vector> updates; + std::vector> updates; auto snapshot = std::make_shared(); snapshot->snapshot_id = 1; snapshot->sequence_number = 1; snapshot->timestamp_ms = TimePointMs{std::chrono::milliseconds(1000)}; snapshot->manifest_list = "s3://bucket/manifest_list"; - updates.push_back(std::make_unique(snapshot)); auto result = TableRequirements::ForUpdateTable(*metadata, updates); ASSERT_THAT(result, IsOk()); auto& requirements = result.value(); - // AddSnapshot doesn't add additional requirements ASSERT_EQ(requirements.size(), 1); EXPECT_EQ(CountRequirementsOfType(requirements), 1); - // Validate against base metadata for (const auto& req : requirements) { EXPECT_THAT(req->Validate(metadata.get()), IsOk()); } } +// RemoveSnapshots Tests + +TEST(TableRequirementsTest, RemoveSnapshots) { + auto metadata = CreateBaseMetadata(); + + std::vector> updates; + updates.push_back(std::make_unique(std::vector{0})); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + ASSERT_EQ(requirements.size(), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } +} + +// SetSnapshotRef Tests + +TEST(TableRequirementsTest, SetSnapshotRef) { + constexpr int64_t kSnapshotId = 14; + const std::string kRefName = "branch"; + + auto metadata = CreateBaseMetadata(); + AddBranch(*metadata, kRefName, kSnapshotId); + + // Multiple updates to same ref should deduplicate + std::vector> updates; + updates.push_back(std::make_unique(kRefName, kSnapshotId, + SnapshotRefType::kBranch)); + updates.push_back(std::make_unique(kRefName, kSnapshotId + 1, + SnapshotRefType::kBranch)); + updates.push_back(std::make_unique(kRefName, kSnapshotId + 2, + SnapshotRefType::kBranch)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + for (const auto& req : requirements) { + EXPECT_THAT(req->Validate(metadata.get()), IsOk()); + } + + ASSERT_EQ(requirements.size(), 2); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + EXPECT_EQ(CountRequirementsOfType(requirements), 1); + + auto* assert_ref = dynamic_cast(requirements[1].get()); + ASSERT_NE(assert_ref, nullptr); + EXPECT_EQ(assert_ref->snapshot_id(), kSnapshotId); + EXPECT_EQ(assert_ref->ref_name(), kRefName); +} + // RemoveSnapshotRef Tests TEST(TableRequirementsTest, RemoveSnapshotRef) { auto metadata = CreateBaseMetadata(); - std::vector> updates; + std::vector> updates; updates.push_back(std::make_unique("branch")); auto result = TableRequirements::ForUpdateTable(*metadata, updates); ASSERT_THAT(result, IsOk()); auto& requirements = result.value(); - // RemoveSnapshotRef doesn't add additional requirements ASSERT_EQ(requirements.size(), 1); EXPECT_EQ(CountRequirementsOfType(requirements), 1); - // Validate against base metadata for (const auto& req : requirements) { EXPECT_THAT(req->Validate(metadata.get()), IsOk()); } From e445919d38b81bc70de622c5b876e84528173952 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Thu, 4 Dec 2025 12:49:35 +0800 Subject: [PATCH 6/6] Update src/iceberg/table_requirements.cc --- src/iceberg/table_requirements.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/iceberg/table_requirements.cc b/src/iceberg/table_requirements.cc index 99ffcdafe..6de6c59e6 100644 --- a/src/iceberg/table_requirements.cc +++ b/src/iceberg/table_requirements.cc @@ -98,7 +98,7 @@ void TableUpdateContext::RequireNoBranchesChanged() { } bool TableUpdateContext::AddChangedRef(const std::string& ref_name) { - auto [it, inserted] = changed_refs_.insert(ref_name); + auto [_, inserted] = changed_refs_.insert(ref_name); return inserted; }