From d3f7caa312ff3a0cd0414dba07dd3095d8df3893 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Mon, 1 Dec 2025 20:04:39 +0800 Subject: [PATCH 1/3] 1 --- src/iceberg/table_requirements.cc | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/iceberg/table_requirements.cc b/src/iceberg/table_requirements.cc index fcb744aad..c9d34c293 100644 --- a/src/iceberg/table_requirements.cc +++ b/src/iceberg/table_requirements.cc @@ -19,7 +19,10 @@ #include "iceberg/table_requirements.h" +#include + #include "iceberg/table_metadata.h" +#include "iceberg/table_requirement.h" #include "iceberg/table_update.h" namespace iceberg { @@ -34,19 +37,34 @@ Result>> TableUpdateContext::Build Result>> TableRequirements::ForCreateTable( const std::vector>& table_updates) { - return NotImplemented("TableRequirements::ForCreateTable not implemented"); + TableUpdateContext context(nullptr, false); + context.AddRequirement(std::make_unique()); + for (const auto& update : table_updates) { + ICEBERG_RETURN_UNEXPECTED(update->GenerateRequirements(context)); + } + return context.Build(); } Result>> TableRequirements::ForReplaceTable( const TableMetadata& base, const std::vector>& table_updates) { - return NotImplemented("TableRequirements::ForReplaceTable not implemented"); + TableUpdateContext context(&base, true); + context.AddRequirement(std::make_unique(base.table_uuid)); + for (const auto& update : table_updates) { + ICEBERG_RETURN_UNEXPECTED(update->GenerateRequirements(context)); + } + return context.Build(); } Result>> TableRequirements::ForUpdateTable( const TableMetadata& base, const std::vector>& table_updates) { - return NotImplemented("TableRequirements::ForUpdateTable not implemented"); + TableUpdateContext context(&base, false); + context.AddRequirement(std::make_unique(base.table_uuid)); + for (const auto& update : table_updates) { + ICEBERG_RETURN_UNEXPECTED(update->GenerateRequirements(context)); + } + return context.Build(); } } // namespace iceberg From 3b15ddff7c86ff53234d76513b09ba2be262dbcf Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Tue, 2 Dec 2025 11:10:01 +0800 Subject: [PATCH 2/3] feat: implement rable_requirements --- src/iceberg/table_requirements.h | 8 + src/iceberg/table_update.cc | 17 +- src/iceberg/test/CMakeLists.txt | 1 + src/iceberg/test/table_requirements_test.cc | 270 ++++++++++++++++++++ src/iceberg/test/table_update_test.cc | 7 +- 5 files changed, 286 insertions(+), 17 deletions(-) create mode 100644 src/iceberg/test/table_requirements_test.cc diff --git a/src/iceberg/table_requirements.h b/src/iceberg/table_requirements.h index 327e7d53f..2e1945ecd 100644 --- a/src/iceberg/table_requirements.h +++ b/src/iceberg/table_requirements.h @@ -73,6 +73,14 @@ class ICEBERG_EXPORT TableUpdateContext { const bool is_replace_; std::vector> requirements_; + + // Deduplication flags to avoid adding duplicate requirements + // These track whether specific requirement types have already been added + bool added_last_assigned_field_id_ = false; + bool added_current_schema_id_ = false; + bool added_last_assigned_partition_id_ = false; + bool added_default_spec_id_ = false; + bool added_default_sort_order_id_ = false; }; /// \brief Factory class for generating table requirements diff --git a/src/iceberg/table_update.cc b/src/iceberg/table_update.cc index fcb7a58c2..b7d367244 100644 --- a/src/iceberg/table_update.cc +++ b/src/iceberg/table_update.cc @@ -33,19 +33,7 @@ void AssignUUID::ApplyTo(TableMetadataBuilder& builder) const { } Status AssignUUID::GenerateRequirements(TableUpdateContext& context) const { - // AssignUUID operation generates a requirement to assert the table's UUID - // if a base metadata exists (i.e., this is an update operation) - - const TableMetadata* base = context.base(); - - if (base != nullptr && !base->table_uuid.empty()) { - // For table updates, assert that the current UUID matches what we expect - context.AddRequirement(std::make_unique(base->table_uuid)); - } - - // Note: For table creation (base == nullptr), no UUID requirement is needed - // as the table doesn't exist yet - + // AssignUUID does not generate additional requirements. return {}; } @@ -56,7 +44,8 @@ void UpgradeFormatVersion::ApplyTo(TableMetadataBuilder& builder) const { } Status UpgradeFormatVersion::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("UpgradeFormatVersion::GenerateRequirements not implemented"); + // AssignUUID does not generate additional requirements. + return {}; } // AddSchema diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index af3dfa0fb..b0857d819 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -86,6 +86,7 @@ add_iceberg_test(table_test table_test.cc table_metadata_builder_test.cc table_requirement_test.cc + table_requirements_test.cc table_update_test.cc test_common.cc) diff --git a/src/iceberg/test/table_requirements_test.cc b/src/iceberg/test/table_requirements_test.cc new file mode 100644 index 000000000..5f820b99f --- /dev/null +++ b/src/iceberg/test/table_requirements_test.cc @@ -0,0 +1,270 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/table_requirements.h" + +#include +#include +#include + +#include + +#include "iceberg/partition_spec.h" +#include "iceberg/snapshot.h" +#include "iceberg/sort_order.h" +#include "iceberg/table_metadata.h" +#include "iceberg/table_requirement.h" +#include "iceberg/table_update.h" +#include "iceberg/test/matchers.h" + +namespace iceberg { + +namespace { + +// Helper function to create base metadata for tests +std::unique_ptr CreateBaseMetadata( + const std::string& uuid = "test-uuid-1234") { + auto metadata = std::make_unique(); + metadata->format_version = 2; + metadata->table_uuid = uuid; + 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->default_spec_id = PartitionSpec::kInitialSpecId; + metadata->last_partition_id = 0; + metadata->current_snapshot_id = Snapshot::kInvalidSnapshotId; + metadata->default_sort_order_id = SortOrder::kInitialSortOrderId; + metadata->next_row_id = TableMetadata::kInitialRowId; + return metadata; +} + +} // namespace + +// Test: Empty updates for CreateTable +TEST(TableRequirementsTest, EmptyUpdatesForCreateTable) { + std::vector> updates; + + auto result = TableRequirements::ForCreateTable(updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + ASSERT_EQ(requirements.size(), 1); + + // Should have only AssertDoesNotExist requirement + auto* assert_does_not_exist = + dynamic_cast(requirements[0].get()); + EXPECT_NE(assert_does_not_exist, nullptr); +} + +// Test: Empty updates for UpdateTable +TEST(TableRequirementsTest, EmptyUpdatesForUpdateTable) { + auto metadata = CreateBaseMetadata(); + std::vector> updates; + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + ASSERT_EQ(requirements.size(), 1); + + // Should have only AssertUUID requirement + auto* assert_uuid = dynamic_cast(requirements[0].get()); + ASSERT_NE(assert_uuid, nullptr); + EXPECT_EQ(assert_uuid->uuid(), metadata->table_uuid); +} + +// Test: Empty updates for ReplaceTable +TEST(TableRequirementsTest, EmptyUpdatesForReplaceTable) { + auto metadata = CreateBaseMetadata(); + std::vector> updates; + + auto result = TableRequirements::ForReplaceTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + ASSERT_EQ(requirements.size(), 1); + + // Should have only AssertUUID requirement + auto* assert_uuid = dynamic_cast(requirements[0].get()); + ASSERT_NE(assert_uuid, nullptr); + EXPECT_EQ(assert_uuid->uuid(), metadata->table_uuid); +} + +// Test: Table already exists (CreateTable requirement validation) +TEST(TableRequirementsTest, TableAlreadyExists) { + std::vector> updates; + + auto result = TableRequirements::ForCreateTable(updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + ASSERT_EQ(requirements.size(), 1); + + // Validate against existing metadata (should fail) + auto metadata = CreateBaseMetadata(); + auto status = requirements[0]->Validate(metadata.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("table already exists")); +} + +// Test: Table does not exist (CreateTable requirement validation) +TEST(TableRequirementsTest, TableDoesNotExist) { + std::vector> updates; + + auto result = TableRequirements::ForCreateTable(updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + ASSERT_EQ(requirements.size(), 1); + + // Validate against null metadata (should succeed) + auto status = requirements[0]->Validate(nullptr); + EXPECT_THAT(status, IsOk()); +} + +// Test: AssignUUID for UpdateTable +TEST(TableRequirementsTest, AssignUUID) { + auto metadata = CreateBaseMetadata("original-uuid"); + std::vector> updates; + + // Add multiple AssignUUID updates + updates.push_back(std::make_unique(metadata->table_uuid)); + updates.push_back(std::make_unique("new-uuid-1")); + updates.push_back(std::make_unique("new-uuid-2")); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // After deduplication: only 1 AssertUUID from ForUpdateTable + ASSERT_EQ(requirements.size(), 1); + + // Should have AssertUUID requirement with original UUID + auto* assert_uuid = dynamic_cast(requirements[0].get()); + ASSERT_NE(assert_uuid, nullptr); + EXPECT_EQ(assert_uuid->uuid(), "original-uuid"); + + // Validate against base metadata (should succeed) + auto status = requirements[0]->Validate(metadata.get()); + EXPECT_THAT(status, IsOk()); +} + +// Test: AssignUUID validation failure +TEST(TableRequirementsTest, AssignUUIDFailure) { + auto metadata = CreateBaseMetadata("original-uuid"); + std::vector> updates; + updates.push_back(std::make_unique(metadata->table_uuid)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // After deduplication: only 1 AssertUUID from ForUpdateTable + ASSERT_EQ(requirements.size(), 1); + + // Create updated metadata with different UUID + auto updated = CreateBaseMetadata("different-uuid"); + + // Validate against updated metadata (should fail) + auto status = requirements[0]->Validate(updated.get()); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("UUID does not match")); +} + +// Test: AssignUUID for ReplaceTable +TEST(TableRequirementsTest, AssignUUIDForReplaceTable) { + auto metadata = CreateBaseMetadata("original-uuid"); + std::vector> updates; + + // Add multiple AssignUUID updates + updates.push_back(std::make_unique(metadata->table_uuid)); + updates.push_back(std::make_unique("new-uuid-1")); + updates.push_back(std::make_unique("new-uuid-2")); + + auto result = TableRequirements::ForReplaceTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // After deduplication: only 1 AssertUUID from ForReplaceTable + ASSERT_EQ(requirements.size(), 1); + + // Should have AssertUUID requirement + auto* assert_uuid = dynamic_cast(requirements[0].get()); + ASSERT_NE(assert_uuid, nullptr); + EXPECT_EQ(assert_uuid->uuid(), "original-uuid"); + + // Validate against base metadata (should succeed) + auto status = requirements[0]->Validate(metadata.get()); + EXPECT_THAT(status, IsOk()); +} + +// Test: Multiple requirements validation +TEST(TableRequirementsTest, ValidateAllRequirements) { + auto metadata = CreateBaseMetadata("test-uuid"); + std::vector> updates; + updates.push_back(std::make_unique(metadata->table_uuid)); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + + // All requirements should validate successfully + for (const auto& req : requirements) { + auto status = req->Validate(metadata.get()); + EXPECT_THAT(status, IsOk()); + } +} + +// Test: UUID comparison is case-insensitive +TEST(TableRequirementsTest, AssignUUIDCaseInsensitive) { + auto metadata = CreateBaseMetadata("TEST-UUID-1234"); + std::vector> updates; + updates.push_back(std::make_unique("test-uuid-1234")); + + auto result = TableRequirements::ForUpdateTable(*metadata, updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + ASSERT_EQ(requirements.size(), 1); + + // Validate against base metadata (should succeed due to case-insensitive comparison) + auto status = requirements[0]->Validate(metadata.get()); + EXPECT_THAT(status, IsOk()); +} + +// Test: ForCreateTable with AssignUUID does not generate requirements +TEST(TableRequirementsTest, CreateTableWithAssignUUID) { + std::vector> updates; + updates.push_back(std::make_unique("new-table-uuid")); + + auto result = TableRequirements::ForCreateTable(updates); + ASSERT_THAT(result, IsOk()); + + auto& requirements = result.value(); + // Should have only AssertDoesNotExist, no UUID requirement for new tables + ASSERT_EQ(requirements.size(), 1); + + auto* assert_does_not_exist = + dynamic_cast(requirements[0].get()); + EXPECT_NE(assert_does_not_exist, nullptr); +} + +} // namespace iceberg diff --git a/src/iceberg/test/table_update_test.cc b/src/iceberg/test/table_update_test.cc index 9607db59f..298141a93 100644 --- a/src/iceberg/test/table_update_test.cc +++ b/src/iceberg/test/table_update_test.cc @@ -71,14 +71,15 @@ std::unique_ptr CreateBaseMetadata() { TEST(TableUpdateTest, AssignUUIDGenerateRequirements) { table::AssignUUID update("new-uuid"); - // New table - no requirements + // New table - no requirements (AssignUUID doesn't generate requirements) auto new_table_reqs = GenerateRequirements(update, nullptr); EXPECT_TRUE(new_table_reqs.empty()); - // Existing table - should generate AssertUUID requirement + // Existing table - AssignUUID doesn't generate requirements anymore + // The UUID assertion is added by ForUpdateTable/ForReplaceTable methods auto base = CreateBaseMetadata(); auto existing_table_reqs = GenerateRequirements(update, base.get()); - EXPECT_EQ(existing_table_reqs.size(), 1); + EXPECT_TRUE(existing_table_reqs.empty()); // Existing table with empty UUID - no requirements base->table_uuid = ""; From 6af2034bc890577882cee2a00c4797762660d23d Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Tue, 2 Dec 2025 11:15:21 +0800 Subject: [PATCH 3/3] update --- src/iceberg/table_requirements.h | 25 ++++++++- src/iceberg/table_update.cc | 3 +- src/iceberg/test/table_requirements_test.cc | 62 +-------------------- 3 files changed, 25 insertions(+), 65 deletions(-) diff --git a/src/iceberg/table_requirements.h b/src/iceberg/table_requirements.h index 2e1945ecd..7af2fb2df 100644 --- a/src/iceberg/table_requirements.h +++ b/src/iceberg/table_requirements.h @@ -68,14 +68,35 @@ 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; + } + private: const TableMetadata* base_; const bool is_replace_; std::vector> requirements_; - // Deduplication flags to avoid adding duplicate requirements - // These track whether specific requirement types have already been added + // flags to avoid adding duplicate requirements bool added_last_assigned_field_id_ = false; bool added_current_schema_id_ = false; bool added_last_assigned_partition_id_ = false; diff --git a/src/iceberg/table_update.cc b/src/iceberg/table_update.cc index b7d367244..71fa30ee0 100644 --- a/src/iceberg/table_update.cc +++ b/src/iceberg/table_update.cc @@ -44,8 +44,7 @@ void UpgradeFormatVersion::ApplyTo(TableMetadataBuilder& builder) const { } Status UpgradeFormatVersion::GenerateRequirements(TableUpdateContext& context) const { - // AssignUUID does not generate additional requirements. - return {}; + return NotImplemented("UpgradeFormatVersion::GenerateRequirements not implemented"); } // AddSchema diff --git a/src/iceberg/test/table_requirements_test.cc b/src/iceberg/test/table_requirements_test.cc index 5f820b99f..441e72d41 100644 --- a/src/iceberg/test/table_requirements_test.cc +++ b/src/iceberg/test/table_requirements_test.cc @@ -57,7 +57,6 @@ std::unique_ptr CreateBaseMetadata( } // namespace -// Test: Empty updates for CreateTable TEST(TableRequirementsTest, EmptyUpdatesForCreateTable) { std::vector> updates; @@ -73,7 +72,6 @@ TEST(TableRequirementsTest, EmptyUpdatesForCreateTable) { EXPECT_NE(assert_does_not_exist, nullptr); } -// Test: Empty updates for UpdateTable TEST(TableRequirementsTest, EmptyUpdatesForUpdateTable) { auto metadata = CreateBaseMetadata(); std::vector> updates; @@ -90,7 +88,6 @@ TEST(TableRequirementsTest, EmptyUpdatesForUpdateTable) { EXPECT_EQ(assert_uuid->uuid(), metadata->table_uuid); } -// Test: Empty updates for ReplaceTable TEST(TableRequirementsTest, EmptyUpdatesForReplaceTable) { auto metadata = CreateBaseMetadata(); std::vector> updates; @@ -107,7 +104,6 @@ TEST(TableRequirementsTest, EmptyUpdatesForReplaceTable) { EXPECT_EQ(assert_uuid->uuid(), metadata->table_uuid); } -// Test: Table already exists (CreateTable requirement validation) TEST(TableRequirementsTest, TableAlreadyExists) { std::vector> updates; @@ -124,7 +120,6 @@ TEST(TableRequirementsTest, TableAlreadyExists) { EXPECT_THAT(status, HasErrorMessage("table already exists")); } -// Test: Table does not exist (CreateTable requirement validation) TEST(TableRequirementsTest, TableDoesNotExist) { std::vector> updates; @@ -139,7 +134,7 @@ TEST(TableRequirementsTest, TableDoesNotExist) { EXPECT_THAT(status, IsOk()); } -// Test: AssignUUID for UpdateTable +// Test for AssignUUID update TEST(TableRequirementsTest, AssignUUID) { auto metadata = CreateBaseMetadata("original-uuid"); std::vector> updates; @@ -161,12 +156,10 @@ TEST(TableRequirementsTest, AssignUUID) { ASSERT_NE(assert_uuid, nullptr); EXPECT_EQ(assert_uuid->uuid(), "original-uuid"); - // Validate against base metadata (should succeed) auto status = requirements[0]->Validate(metadata.get()); EXPECT_THAT(status, IsOk()); } -// Test: AssignUUID validation failure TEST(TableRequirementsTest, AssignUUIDFailure) { auto metadata = CreateBaseMetadata("original-uuid"); std::vector> updates; @@ -188,7 +181,6 @@ TEST(TableRequirementsTest, AssignUUIDFailure) { EXPECT_THAT(status, HasErrorMessage("UUID does not match")); } -// Test: AssignUUID for ReplaceTable TEST(TableRequirementsTest, AssignUUIDForReplaceTable) { auto metadata = CreateBaseMetadata("original-uuid"); std::vector> updates; @@ -215,56 +207,4 @@ TEST(TableRequirementsTest, AssignUUIDForReplaceTable) { EXPECT_THAT(status, IsOk()); } -// Test: Multiple requirements validation -TEST(TableRequirementsTest, ValidateAllRequirements) { - auto metadata = CreateBaseMetadata("test-uuid"); - std::vector> updates; - updates.push_back(std::make_unique(metadata->table_uuid)); - - auto result = TableRequirements::ForUpdateTable(*metadata, updates); - ASSERT_THAT(result, IsOk()); - - auto& requirements = result.value(); - - // All requirements should validate successfully - for (const auto& req : requirements) { - auto status = req->Validate(metadata.get()); - EXPECT_THAT(status, IsOk()); - } -} - -// Test: UUID comparison is case-insensitive -TEST(TableRequirementsTest, AssignUUIDCaseInsensitive) { - auto metadata = CreateBaseMetadata("TEST-UUID-1234"); - std::vector> updates; - updates.push_back(std::make_unique("test-uuid-1234")); - - auto result = TableRequirements::ForUpdateTable(*metadata, updates); - ASSERT_THAT(result, IsOk()); - - auto& requirements = result.value(); - ASSERT_EQ(requirements.size(), 1); - - // Validate against base metadata (should succeed due to case-insensitive comparison) - auto status = requirements[0]->Validate(metadata.get()); - EXPECT_THAT(status, IsOk()); -} - -// Test: ForCreateTable with AssignUUID does not generate requirements -TEST(TableRequirementsTest, CreateTableWithAssignUUID) { - std::vector> updates; - updates.push_back(std::make_unique("new-table-uuid")); - - auto result = TableRequirements::ForCreateTable(updates); - ASSERT_THAT(result, IsOk()); - - auto& requirements = result.value(); - // Should have only AssertDoesNotExist, no UUID requirement for new tables - ASSERT_EQ(requirements.size(), 1); - - auto* assert_does_not_exist = - dynamic_cast(requirements[0].get()); - EXPECT_NE(assert_does_not_exist, nullptr); -} - } // namespace iceberg