From 9825bd386f9acf2e0d3d2f040a3a87109eac86bf Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Tue, 21 Oct 2025 00:01:45 +0800 Subject: [PATCH 1/5] feat: implement TableMetadataBuilder with AssignUUID This commit implements the core TableMetadataBuilder pattern following the established design from the previous session. Pattern established: 1. Builder validates input and collects errors (no exceptions) 2. Changes are tracked for serialization 3. Requirements generated for optimistic concurrency 4. Build() validates metadata consistency before returning --- src/iceberg/table_metadata.cc | 113 ++++++- src/iceberg/table_metadata.h | 3 + src/iceberg/table_requirement.cc | 17 +- src/iceberg/table_requirements.cc | 4 +- src/iceberg/table_update.cc | 21 +- src/iceberg/test/CMakeLists.txt | 2 + src/iceberg/test/meson.build | 3 + .../test/table_metadata_builder_test.cc | 311 ++++++++++++++++++ 8 files changed, 458 insertions(+), 16 deletions(-) create mode 100644 src/iceberg/test/table_metadata_builder_test.cc diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index e32e75ee1..d9b71dcd0 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -20,6 +20,7 @@ #include "iceberg/table_metadata.h" #include +#include #include #include @@ -36,6 +37,7 @@ #include "iceberg/table_update.h" #include "iceberg/util/gzip_internal.h" #include "iceberg/util/macros.h" +#include "iceberg/util/uuid.h" namespace iceberg { @@ -201,13 +203,46 @@ Status TableMetadataUtil::Write(FileIO& io, const std::string& location, // TableMetadataBuilder implementation -struct TableMetadataBuilder::Impl {}; +struct TableMetadataBuilder::Impl { + // Base metadata (nullptr for new tables) + const TableMetadata* base; + + // Working metadata copy + TableMetadata metadata; + + // Change tracking + std::vector> changes; + + // Error collection (since methods return *this and cannot throw) + std::vector errors; + + // Metadata location tracking + std::optional metadata_location; + std::optional previous_metadata_location; + + // Constructor for new table + explicit Impl(int8_t format_version) : base(nullptr), metadata{} { + metadata.format_version = format_version; + metadata.last_sequence_number = TableMetadata::kInitialSequenceNumber; + metadata.last_updated_ms = TimePointMs{std::chrono::milliseconds(0)}; + metadata.last_column_id = 0; + metadata.default_spec_id = TableMetadata::kInitialSpecId; + metadata.last_partition_id = 0; + metadata.current_snapshot_id = TableMetadata::kInvalidSnapshotId; + metadata.default_sort_order_id = TableMetadata::kInitialSortOrderId; + metadata.next_row_id = TableMetadata::kInitialRowId; + } + + // Constructor from existing metadata + explicit Impl(const TableMetadata* base_metadata) + : base(base_metadata), metadata(*base_metadata) {} +}; TableMetadataBuilder::TableMetadataBuilder(int8_t format_version) - : impl_(std::make_unique()) {} + : impl_(std::make_unique(format_version)) {} TableMetadataBuilder::TableMetadataBuilder(const TableMetadata* base) - : impl_(std::make_unique()) {} + : impl_(std::make_unique(base)) {} TableMetadataBuilder::~TableMetadataBuilder() = default; @@ -238,12 +273,35 @@ TableMetadataBuilder& TableMetadataBuilder::SetPreviousMetadataLocation( } TableMetadataBuilder& TableMetadataBuilder::AssignUUID() { - throw IcebergError(std::format("{} not implemented", __FUNCTION__)); + if (impl_->metadata.table_uuid.empty()) { + // Generate a random UUID + return AssignUUID(Uuid::GenerateV4().ToString()); + } + + return *this; } TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) { - throw IcebergError(std::format("{} not implemented", __FUNCTION__)); - ; + std::string uuid_str(uuid); + + // Validation: UUID cannot be null or empty + if (uuid_str.empty()) { + impl_->errors.emplace_back(InvalidArgument("Cannot assign null or empty UUID")); + return *this; + } + + // Check if UUID is already set to the same value (no-op) + if (StringUtils::EqualsIgnoreCase(impl_->metadata.table_uuid, uuid_str)) { + return *this; + } + + // Update the metadata + impl_->metadata.table_uuid = uuid_str; + + // Record the change + impl_->changes.push_back(std::make_unique(uuid_str)); + + return *this; } TableMetadataBuilder& TableMetadataBuilder::UpgradeFormatVersion( @@ -378,11 +436,50 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveEncryptionKey(std::string_view } TableMetadataBuilder& TableMetadataBuilder::DiscardChanges() { - throw IcebergError(std::format("{} not implemented", __FUNCTION__)); + // Clear all changes and errors + impl_->changes.clear(); + impl_->errors.clear(); + + // Reset metadata to base state + if (impl_->base != nullptr) { + impl_->metadata = *impl_->base; + } else { + // Reset to initial state for new table + *impl_ = Impl(impl_->metadata.format_version); + } + + return *this; } Result> TableMetadataBuilder::Build() { - return NotImplemented("TableMetadataBuilder::Build not implemented"); + // 1. Check for accumulated errors + if (!impl_->errors.empty()) { + std::string error_msg = "Failed to build TableMetadata due to validation errors:\n"; + for (const auto& error : impl_->errors) { + error_msg += " - " + error.error().message + "\n"; + } + return CommitFailed("{}", error_msg); + } + + // 2. Validate metadata consistency + + // Validate UUID exists for format version > 1 + if (impl_->metadata.format_version > 1 && impl_->metadata.table_uuid.empty()) { + return InvalidArgument("UUID is required for format version {}", + impl_->metadata.format_version); + } + + // 3. Update last_updated_ms if there are changes + if (!impl_->changes.empty() && impl_->base != nullptr) { + impl_->metadata.last_updated_ms = + TimePointMs{std::chrono::duration_cast( + std::chrono::system_clock::now().time_since_epoch())}; + } + + // 4. Create and return the TableMetadata + auto result = std::make_unique(std::move(impl_->metadata)); + + return result; } } // namespace iceberg diff --git a/src/iceberg/table_metadata.h b/src/iceberg/table_metadata.h index 11b17eb84..02e9a9b7d 100644 --- a/src/iceberg/table_metadata.h +++ b/src/iceberg/table_metadata.h @@ -73,6 +73,9 @@ struct ICEBERG_EXPORT TableMetadata { static constexpr int64_t kInitialSequenceNumber = 0; static constexpr int64_t kInvalidSequenceNumber = -1; static constexpr int64_t kInitialRowId = 0; + static constexpr int32_t kInitialSpecId = 0; + static constexpr int32_t kInitialSortOrderId = 1; + static constexpr int64_t kInvalidSnapshotId = -1; /// An integer version number for the format int8_t format_version; diff --git a/src/iceberg/table_requirement.cc b/src/iceberg/table_requirement.cc index 4ca4b915f..1ae07c303 100644 --- a/src/iceberg/table_requirement.cc +++ b/src/iceberg/table_requirement.cc @@ -20,15 +20,28 @@ #include "iceberg/table_requirement.h" #include "iceberg/table_metadata.h" +#include "util/string_util.h" namespace iceberg::table { Status AssertDoesNotExist::Validate(const TableMetadata* base) const { - return NotImplemented("AssertTableDoesNotExist::Validate not implemented"); + return NotImplemented("AssertDoesNotExist::Validate not implemented"); } Status AssertUUID::Validate(const TableMetadata* base) const { - return NotImplemented("AssertTableUUID::Validate not implemented"); + // Validate that the table UUID matches the expected value + + if (base == nullptr) { + return CommitFailed("Requirement failed: current table metadata is missing"); + } + + if (!StringUtils::EqualsIgnoreCase(base->table_uuid, uuid_)) { + return CommitFailed( + "Requirement failed: table UUID does not match (expected='{}', actual='{}')", + uuid_, base->table_uuid); + } + + return {}; } Status AssertRefSnapshotID::Validate(const TableMetadata* base) const { diff --git a/src/iceberg/table_requirements.cc b/src/iceberg/table_requirements.cc index 1eb870cb4..aae874ecd 100644 --- a/src/iceberg/table_requirements.cc +++ b/src/iceberg/table_requirements.cc @@ -26,11 +26,11 @@ namespace iceberg { void TableUpdateContext::AddRequirement(std::unique_ptr requirement) { - throw IcebergError("TableUpdateContext::AddRequirement not implemented"); + requirements_.emplace_back(std::move(requirement)); } Result>> TableUpdateContext::Build() { - return NotImplemented("TableUpdateContext::Build not implemented"); + return std::move(requirements_); } Result>> TableRequirements::ForCreateTable( diff --git a/src/iceberg/table_update.cc b/src/iceberg/table_update.cc index 7d81dd8f1..fcb7a58c2 100644 --- a/src/iceberg/table_update.cc +++ b/src/iceberg/table_update.cc @@ -21,6 +21,7 @@ #include "iceberg/exception.h" #include "iceberg/table_metadata.h" +#include "iceberg/table_requirement.h" #include "iceberg/table_requirements.h" namespace iceberg::table { @@ -28,11 +29,24 @@ namespace iceberg::table { // AssignUUID void AssignUUID::ApplyTo(TableMetadataBuilder& builder) const { - throw IcebergError(std::format("{} not implemented", __FUNCTION__)); + builder.AssignUUID(uuid_); } Status AssignUUID::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented("AssignTableUUID::GenerateRequirements not implemented"); + // 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 + + return {}; } // UpgradeFormatVersion @@ -42,8 +56,7 @@ void UpgradeFormatVersion::ApplyTo(TableMetadataBuilder& builder) const { } Status UpgradeFormatVersion::GenerateRequirements(TableUpdateContext& context) const { - return NotImplemented( - "UpgradeTableFormatVersion::GenerateRequirements not implemented"); + return NotImplemented("UpgradeFormatVersion::GenerateRequirements not implemented"); } // AddSchema diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 68af62bf1..c9f7a032b 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -84,6 +84,8 @@ add_iceberg_test(table_test table_test.cc schema_json_test.cc) +add_iceberg_test(table_metadata_builder_test SOURCES table_metadata_builder_test.cc) + add_iceberg_test(expression_test SOURCES expression_test.cc diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index 88b16325c..189c6310c 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -80,6 +80,9 @@ iceberg_tests = { ), }, 'roaring_test': {'sources': files('roaring_test.cc')}, + 'table_metadata_builder_test': { + 'sources': files('table_metadata_builder_test.cc'), + }, } if get_option('rest').enabled() diff --git a/src/iceberg/test/table_metadata_builder_test.cc b/src/iceberg/test/table_metadata_builder_test.cc new file mode 100644 index 000000000..4979ea34d --- /dev/null +++ b/src/iceberg/test/table_metadata_builder_test.cc @@ -0,0 +1,311 @@ +/* + * 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 +#include + +#include + +#include "gmock/gmock-matchers.h" +#include "iceberg/table_metadata.h" +#include "iceberg/table_requirement.h" +#include "iceberg/table_requirements.h" +#include "iceberg/table_update.h" +#include "iceberg/test/matchers.h" + +namespace iceberg { + +// Helper functions to reduce test boilerplate +namespace { + +// Build and assert success, returning the metadata +std::unique_ptr BuildAndExpectSuccess(TableMetadataBuilder& builder) { + auto result = builder.Build(); + EXPECT_TRUE(result.has_value()) << "Build failed: " << result.error().message; + if (!result.has_value()) { + return nullptr; + } + return std::move(result.value()); +} + +// Build and expect failure with message containing substring +void BuildAndExpectFailure(TableMetadataBuilder& builder, + const std::string& expected_message_substr) { + auto result = builder.Build(); + ASSERT_FALSE(result.has_value()) << "Build should have failed"; + EXPECT_TRUE(result.error().message.find(expected_message_substr) != std::string::npos) + << "Expected error message to contain: " << expected_message_substr + << "\nActual error: " << result.error().message; +} + +// Generate requirements and return them +std::vector> GenerateRequirements( + const TableUpdate& update, const TableMetadata* base) { + TableUpdateContext context(base, false); + auto status = update.GenerateRequirements(context); + EXPECT_TRUE(status.has_value()) + << "GenerateRequirements failed: " << status.error().message; + + auto requirements = context.Build(); + EXPECT_TRUE(requirements.has_value()); + + if (!requirements.has_value()) { + return {}; + } + return std::move(requirements.value()); +} + +} // namespace + +// Test fixture for TableMetadataBuilder tests +class TableMetadataBuilderTest : public ::testing::Test { + protected: + void SetUp() override { + // Create a base metadata for update tests + base_metadata_ = std::make_unique(); + base_metadata_->format_version = 2; + base_metadata_->table_uuid = "test-uuid-1234"; + base_metadata_->location = "s3://bucket/test"; + base_metadata_->last_sequence_number = 0; + base_metadata_->last_updated_ms = TimePointMs{std::chrono::milliseconds(1000)}; + base_metadata_->last_column_id = 0; + base_metadata_->default_spec_id = TableMetadata::kInitialSpecId; + base_metadata_->last_partition_id = 0; + base_metadata_->current_snapshot_id = TableMetadata::kInvalidSnapshotId; + base_metadata_->default_sort_order_id = TableMetadata::kInitialSortOrderId; + base_metadata_->next_row_id = TableMetadata::kInitialRowId; + } + + std::unique_ptr base_metadata_; +}; + +// ============================================================================ +// TableMetadataBuilder - Basic Construction Tests +// ============================================================================ + +TEST_F(TableMetadataBuilderTest, BuildFromEmpty) { + auto builder = TableMetadataBuilder::BuildFromEmpty(2); + ASSERT_NE(builder, nullptr); + + builder->AssignUUID("new-uuid-5678"); + + auto metadata = BuildAndExpectSuccess(*builder); + ASSERT_NE(metadata, nullptr); + + EXPECT_EQ(metadata->format_version, 2); + EXPECT_EQ(metadata->last_sequence_number, TableMetadata::kInitialSequenceNumber); + EXPECT_EQ(metadata->default_spec_id, TableMetadata::kInitialSpecId); + EXPECT_EQ(metadata->default_sort_order_id, TableMetadata::kInitialSortOrderId); + EXPECT_EQ(metadata->current_snapshot_id, TableMetadata::kInvalidSnapshotId); +} + +TEST_F(TableMetadataBuilderTest, BuildFromExisting) { + auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); + ASSERT_NE(builder, nullptr); + + auto metadata = BuildAndExpectSuccess(*builder); + ASSERT_NE(metadata, nullptr); + + EXPECT_EQ(metadata->format_version, 2); + EXPECT_EQ(metadata->table_uuid, "test-uuid-1234"); + EXPECT_EQ(metadata->location, "s3://bucket/test"); +} + +// ============================================================================ +// TableMetadataBuilder - AssignUUID Tests +// ============================================================================ + +TEST_F(TableMetadataBuilderTest, AssignUUIDForNewTable) { + auto builder = TableMetadataBuilder::BuildFromEmpty(2); + builder->AssignUUID("new-uuid-5678"); + + auto metadata = BuildAndExpectSuccess(*builder); + EXPECT_EQ(metadata->table_uuid, "new-uuid-5678"); +} + +TEST_F(TableMetadataBuilderTest, AssignUUIDAndUpdateExisting) { + auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); + builder->AssignUUID("updated-uuid-9999"); + + auto metadata = BuildAndExpectSuccess(*builder); + EXPECT_EQ(metadata->table_uuid, "updated-uuid-9999"); +} + +TEST_F(TableMetadataBuilderTest, AssignUUIDWithEmptyUUID) { + auto builder = TableMetadataBuilder::BuildFromEmpty(2); + builder->AssignUUID(""); + + BuildAndExpectFailure(*builder, "Cannot assign null or empty UUID"); +} + +TEST_F(TableMetadataBuilderTest, AssignUUIDWithSameUUID) { + auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); + builder->AssignUUID("test-uuid-1234"); // Same UUID + + auto metadata = BuildAndExpectSuccess(*builder); + EXPECT_EQ(metadata->table_uuid, "test-uuid-1234"); +} + +TEST_F(TableMetadataBuilderTest, AssignUUIDWithAutoGenerate) { + auto builder = TableMetadataBuilder::BuildFromEmpty(2); + builder->AssignUUID(); // Auto-generate + + auto metadata = BuildAndExpectSuccess(*builder); + EXPECT_FALSE(metadata->table_uuid.empty()); +} + +TEST_F(TableMetadataBuilderTest, AssignUUIDAndCaseInsensitiveComparison) { + base_metadata_->table_uuid = "TEST-UUID-ABCD"; + auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); + builder->AssignUUID("test-uuid-abcd"); // Different case - should be no-op + + auto metadata = BuildAndExpectSuccess(*builder); + EXPECT_EQ(metadata->table_uuid, "TEST-UUID-ABCD"); // Original case preserved +} + +// ============================================================================ +// TableMetadataBuilder - DiscardChanges Tests +// ============================================================================ + +TEST_F(TableMetadataBuilderTest, DiscardChangesAndResetsToBase) { + auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); + builder->AssignUUID("new-uuid"); + builder->DiscardChanges(); + + auto metadata = BuildAndExpectSuccess(*builder); + EXPECT_EQ(metadata->table_uuid, "test-uuid-1234"); // Original UUID +} + +// ============================================================================ +// TableUpdate - ApplyTo Tests +// ============================================================================ + +TEST_F(TableMetadataBuilderTest, TableUpdateWithAssignUUID) { + auto builder = TableMetadataBuilder::BuildFromEmpty(2); + + table::AssignUUID update("apply-uuid"); + update.ApplyTo(*builder); + + auto metadata = BuildAndExpectSuccess(*builder); + EXPECT_EQ(metadata->table_uuid, "apply-uuid"); +} + +// ============================================================================ +// TableUpdate - GenerateRequirements Tests +// ============================================================================ + +TEST_F(TableMetadataBuilderTest, + TableUpdateWithAssignUUIDAndGenerateRequirementsForNewTable) { + table::AssignUUID update("new-uuid"); + + auto requirements = GenerateRequirements(update, nullptr); + EXPECT_TRUE(requirements.empty()); // No requirements for new table +} + +TEST_F(TableMetadataBuilderTest, + TableUpdateWithAssignUUIDAndGenerateRequirementsForExistingTable) { + table::AssignUUID update("new-uuid"); + + auto requirements = GenerateRequirements(update, base_metadata_.get()); + EXPECT_EQ(requirements.size(), 1); // Should generate AssertUUID requirement +} + +TEST_F(TableMetadataBuilderTest, + TableUpdateWithAssignUUIDAndGenerateRequirementsWithEmptyUUID) { + base_metadata_->table_uuid = ""; + table::AssignUUID update("new-uuid"); + + auto requirements = GenerateRequirements(update, base_metadata_.get()); + EXPECT_TRUE(requirements.empty()); // No requirement when base has no UUID +} + +// ============================================================================ +// TableRequirement - Validate Tests +// ============================================================================ + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertUUIDSuccess) { + table::AssertUUID requirement("test-uuid-1234"); + + auto status = requirement.Validate(base_metadata_.get()); + + EXPECT_TRUE(status.has_value()) << "Validation failed: " << status.error().message; +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertUUIDMismatch) { + table::AssertUUID requirement("wrong-uuid"); + + auto status = requirement.Validate(base_metadata_.get()); + + ASSERT_FALSE(status.has_value()); + EXPECT_EQ(status.error().kind, ErrorKind::kCommitFailed); + EXPECT_TRUE(status.error().message.find("UUID does not match") != std::string::npos); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertUUIDNullBase) { + table::AssertUUID requirement("any-uuid"); + + auto status = requirement.Validate(nullptr); + + ASSERT_FALSE(status.has_value()); + EXPECT_EQ(status.error().kind, ErrorKind::kCommitFailed); + EXPECT_TRUE(status.error().message.find("metadata is missing") != std::string::npos); +} + +TEST_F(TableMetadataBuilderTest, TableRequirementAssertUUIDCaseInsensitive) { + base_metadata_->table_uuid = "TEST-UUID-1234"; + table::AssertUUID requirement("test-uuid-1234"); + + auto status = requirement.Validate(base_metadata_.get()); + + EXPECT_TRUE(status.has_value()) + << "Validation should succeed with case-insensitive match"; +} + +// ============================================================================ +// Integration Tests - End-to-End Workflow +// ============================================================================ + +TEST_F(TableMetadataBuilderTest, IntegrationCreateTableWithUUID) { + auto builder = TableMetadataBuilder::BuildFromEmpty(2); + builder->AssignUUID("integration-test-uuid"); + + auto metadata = BuildAndExpectSuccess(*builder); + EXPECT_EQ(metadata->table_uuid, "integration-test-uuid"); + EXPECT_EQ(metadata->format_version, 2); +} + +TEST_F(TableMetadataBuilderTest, IntegrationOptimisticConcurrencyControl) { + table::AssignUUID update("new-uuid"); + + // Generate and validate requirements + auto requirements = GenerateRequirements(update, base_metadata_.get()); + for (const auto& req : requirements) { + auto val_status = req->Validate(base_metadata_.get()); + ASSERT_THAT(val_status, IsOk()) << "Requirement validation failed"; + } + + // Apply update and build + auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); + update.ApplyTo(*builder); + + auto metadata = BuildAndExpectSuccess(*builder); + ASSERT_NE(metadata, nullptr); +} + +} // namespace iceberg From 155ae5b6a4873a3f9e0ac0c406af34dcac305e03 Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Wed, 22 Oct 2025 00:03:02 +0800 Subject: [PATCH 2/5] resolve review comments --- src/iceberg/partition_spec.h | 1 + src/iceberg/schema.h | 1 + src/iceberg/table_metadata.cc | 50 +++------ src/iceberg/table_metadata.h | 10 -- src/iceberg/table_requirement.cc | 2 +- src/iceberg/test/CMakeLists.txt | 5 +- src/iceberg/test/matchers.h | 14 +++ src/iceberg/test/meson.build | 4 +- .../test/table_metadata_builder_test.cc | 104 +++++------------- src/iceberg/util/string_util.h | 2 +- 10 files changed, 64 insertions(+), 129 deletions(-) diff --git a/src/iceberg/partition_spec.h b/src/iceberg/partition_spec.h index 554692264..88a081bf7 100644 --- a/src/iceberg/partition_spec.h +++ b/src/iceberg/partition_spec.h @@ -47,6 +47,7 @@ class ICEBERG_EXPORT PartitionSpec : public util::Formattable { /// \brief The start ID for partition field. It is only used to generate /// partition field id for v1 metadata where it is tracked. static constexpr int32_t kLegacyPartitionDataIdStart = 1000; + static constexpr int32_t kInvalidPartitionFieldId = -1; /// \brief Create a new partition spec. /// diff --git a/src/iceberg/schema.h b/src/iceberg/schema.h index 81f9aa394..3e71fa2e7 100644 --- a/src/iceberg/schema.h +++ b/src/iceberg/schema.h @@ -46,6 +46,7 @@ namespace iceberg { class ICEBERG_EXPORT Schema : public StructType { public: static constexpr int32_t kInitialSchemaId = 0; + static constexpr int32_t kInvalidColumnId = -1; explicit Schema(std::vector fields, std::optional schema_id = std::nullopt); diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index d9b71dcd0..04475eebe 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -214,7 +214,7 @@ struct TableMetadataBuilder::Impl { std::vector> changes; // Error collection (since methods return *this and cannot throw) - std::vector errors; + std::vector errors; // Metadata location tracking std::optional metadata_location; @@ -224,12 +224,12 @@ struct TableMetadataBuilder::Impl { explicit Impl(int8_t format_version) : base(nullptr), metadata{} { metadata.format_version = format_version; metadata.last_sequence_number = TableMetadata::kInitialSequenceNumber; - metadata.last_updated_ms = TimePointMs{std::chrono::milliseconds(0)}; - metadata.last_column_id = 0; - metadata.default_spec_id = TableMetadata::kInitialSpecId; - metadata.last_partition_id = 0; - metadata.current_snapshot_id = TableMetadata::kInvalidSnapshotId; - metadata.default_sort_order_id = TableMetadata::kInitialSortOrderId; + metadata.last_updated_ms = TimePointMs::min(); + metadata.last_column_id = Schema::kInvalidColumnId; + metadata.default_spec_id = PartitionSpec::kInitialSpecId; + metadata.last_partition_id = PartitionSpec::kInvalidPartitionFieldId; + metadata.current_snapshot_id = Snapshot::kInvalidSnapshotId; + metadata.default_sort_order_id = SortOrder::kInitialSortOrderId; metadata.next_row_id = TableMetadata::kInitialRowId; } @@ -284,9 +284,9 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID() { TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) { std::string uuid_str(uuid); - // Validation: UUID cannot be null or empty + // Validation: UUID cannot be empty if (uuid_str.empty()) { - impl_->errors.emplace_back(InvalidArgument("Cannot assign null or empty UUID")); + impl_->errors.emplace_back(ErrorKind::kInvalidArgument, "Cannot assign empty UUID"); return *this; } @@ -299,7 +299,7 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) { impl_->metadata.table_uuid = uuid_str; // Record the change - impl_->changes.push_back(std::make_unique(uuid_str)); + impl_->changes.push_back(std::make_unique(std::move(uuid_str))); return *this; } @@ -435,42 +435,20 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveEncryptionKey(std::string_view throw IcebergError(std::format("{} not implemented", __FUNCTION__)); } -TableMetadataBuilder& TableMetadataBuilder::DiscardChanges() { - // Clear all changes and errors - impl_->changes.clear(); - impl_->errors.clear(); - - // Reset metadata to base state - if (impl_->base != nullptr) { - impl_->metadata = *impl_->base; - } else { - // Reset to initial state for new table - *impl_ = Impl(impl_->metadata.format_version); - } - - return *this; -} - Result> TableMetadataBuilder::Build() { // 1. Check for accumulated errors if (!impl_->errors.empty()) { std::string error_msg = "Failed to build TableMetadata due to validation errors:\n"; - for (const auto& error : impl_->errors) { - error_msg += " - " + error.error().message + "\n"; + for (const auto& [kind, message] : impl_->errors) { + error_msg += " - " + message + "\n"; } return CommitFailed("{}", error_msg); } - // 2. Validate metadata consistency - - // Validate UUID exists for format version > 1 - if (impl_->metadata.format_version > 1 && impl_->metadata.table_uuid.empty()) { - return InvalidArgument("UUID is required for format version {}", - impl_->metadata.format_version); - } + // 2. Validate metadata consistency through TableMetadata#Validate // 3. Update last_updated_ms if there are changes - if (!impl_->changes.empty() && impl_->base != nullptr) { + if (impl_->metadata.last_updated_ms == TimePointMs::min()) { impl_->metadata.last_updated_ms = TimePointMs{std::chrono::duration_cast( std::chrono::system_clock::now().time_since_epoch())}; diff --git a/src/iceberg/table_metadata.h b/src/iceberg/table_metadata.h index 02e9a9b7d..2a998c7a1 100644 --- a/src/iceberg/table_metadata.h +++ b/src/iceberg/table_metadata.h @@ -73,9 +73,6 @@ struct ICEBERG_EXPORT TableMetadata { static constexpr int64_t kInitialSequenceNumber = 0; static constexpr int64_t kInvalidSequenceNumber = -1; static constexpr int64_t kInitialRowId = 0; - static constexpr int32_t kInitialSpecId = 0; - static constexpr int32_t kInitialSortOrderId = 1; - static constexpr int64_t kInvalidSnapshotId = -1; /// An integer version number for the format int8_t format_version; @@ -380,13 +377,6 @@ class ICEBERG_EXPORT TableMetadataBuilder { /// \return Reference to this builder for method chaining TableMetadataBuilder& RemoveEncryptionKey(std::string_view key_id); - /// \brief Discard all accumulated changes - /// - /// This is useful when you want to reset the builder state without - /// creating a new builder instance. - /// \return Reference to this builder for method chaining - TableMetadataBuilder& DiscardChanges(); - /// \brief Build the TableMetadata object /// /// \return A Result containing the constructed TableMetadata or an error diff --git a/src/iceberg/table_requirement.cc b/src/iceberg/table_requirement.cc index 1ae07c303..e951d7003 100644 --- a/src/iceberg/table_requirement.cc +++ b/src/iceberg/table_requirement.cc @@ -20,7 +20,7 @@ #include "iceberg/table_requirement.h" #include "iceberg/table_metadata.h" -#include "util/string_util.h" +#include "iceberg/util/string_util.h" namespace iceberg::table { diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index c9f7a032b..100287ff9 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -82,9 +82,8 @@ add_iceberg_test(table_test test_common.cc json_internal_test.cc table_test.cc - schema_json_test.cc) - -add_iceberg_test(table_metadata_builder_test SOURCES table_metadata_builder_test.cc) + schema_json_test.cc + table_metadata_builder_test.cc) add_iceberg_test(expression_test SOURCES diff --git a/src/iceberg/test/matchers.h b/src/iceberg/test/matchers.h index f04d4a51b..55d29be60 100644 --- a/src/iceberg/test/matchers.h +++ b/src/iceberg/test/matchers.h @@ -23,6 +23,7 @@ #include #include "iceberg/result.h" +#include "iceberg/util/macros.h" /* * \brief Define custom matchers for expected values @@ -210,4 +211,17 @@ auto ErrorIs(MatcherT&& matcher) { ResultMatcher>(false, std::forward(matcher))); } +// Evaluate `rexpr` which should return a Result. +// On success: assign the contained value to `lhs`. +// On failure: fail the test with the error message. +#define ICEBERG_UNWRAP_OR_FAIL_IMPL(result_name, lhs, rexpr) \ + auto&& result_name = (rexpr); \ + ASSERT_TRUE(result_name.has_value()) \ + << "Operation failed: " << result_name.error().message; \ + lhs = std::move(result_name.value()); + +#define ICEBERG_UNWRAP_OR_FAIL(lhs, rexpr) \ + ICEBERG_UNWRAP_OR_FAIL_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \ + rexpr) + } // namespace iceberg diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index 189c6310c..3496c6a14 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -49,6 +49,7 @@ iceberg_tests = { 'schema_json_test.cc', 'table_test.cc', 'test_common.cc', + 'table_metadata_builder_test.cc', ), }, 'expression_test': { @@ -80,9 +81,6 @@ iceberg_tests = { ), }, 'roaring_test': {'sources': files('roaring_test.cc')}, - 'table_metadata_builder_test': { - 'sources': files('table_metadata_builder_test.cc'), - }, } if get_option('rest').enabled() diff --git a/src/iceberg/test/table_metadata_builder_test.cc b/src/iceberg/test/table_metadata_builder_test.cc index 4979ea34d..aa22f8571 100644 --- a/src/iceberg/test/table_metadata_builder_test.cc +++ b/src/iceberg/test/table_metadata_builder_test.cc @@ -20,9 +20,11 @@ #include #include +#include #include -#include "gmock/gmock-matchers.h" +#include "iceberg/partition_spec.h" +#include "iceberg/sort_order.h" #include "iceberg/table_metadata.h" #include "iceberg/table_requirement.h" #include "iceberg/table_requirements.h" @@ -34,40 +36,14 @@ namespace iceberg { // Helper functions to reduce test boilerplate namespace { -// Build and assert success, returning the metadata -std::unique_ptr BuildAndExpectSuccess(TableMetadataBuilder& builder) { - auto result = builder.Build(); - EXPECT_TRUE(result.has_value()) << "Build failed: " << result.error().message; - if (!result.has_value()) { - return nullptr; - } - return std::move(result.value()); -} - -// Build and expect failure with message containing substring -void BuildAndExpectFailure(TableMetadataBuilder& builder, - const std::string& expected_message_substr) { - auto result = builder.Build(); - ASSERT_FALSE(result.has_value()) << "Build should have failed"; - EXPECT_TRUE(result.error().message.find(expected_message_substr) != std::string::npos) - << "Expected error message to contain: " << expected_message_substr - << "\nActual error: " << result.error().message; -} - // Generate requirements and return them std::vector> GenerateRequirements( const TableUpdate& update, const TableMetadata* base) { - TableUpdateContext context(base, false); - auto status = update.GenerateRequirements(context); - EXPECT_TRUE(status.has_value()) - << "GenerateRequirements failed: " << status.error().message; + TableUpdateContext context(base, /*is_replace=*/false); + EXPECT_THAT(update.GenerateRequirements(context), IsOk()); auto requirements = context.Build(); - EXPECT_TRUE(requirements.has_value()); - - if (!requirements.has_value()) { - return {}; - } + EXPECT_THAT(requirements, IsOk()); return std::move(requirements.value()); } @@ -85,10 +61,10 @@ class TableMetadataBuilderTest : public ::testing::Test { base_metadata_->last_sequence_number = 0; base_metadata_->last_updated_ms = TimePointMs{std::chrono::milliseconds(1000)}; base_metadata_->last_column_id = 0; - base_metadata_->default_spec_id = TableMetadata::kInitialSpecId; + base_metadata_->default_spec_id = PartitionSpec::kInitialSpecId; base_metadata_->last_partition_id = 0; - base_metadata_->current_snapshot_id = TableMetadata::kInvalidSnapshotId; - base_metadata_->default_sort_order_id = TableMetadata::kInitialSortOrderId; + base_metadata_->current_snapshot_id = Snapshot::kInvalidSnapshotId; + base_metadata_->default_sort_order_id = SortOrder::kInitialSortOrderId; base_metadata_->next_row_id = TableMetadata::kInitialRowId; } @@ -105,21 +81,21 @@ TEST_F(TableMetadataBuilderTest, BuildFromEmpty) { builder->AssignUUID("new-uuid-5678"); - auto metadata = BuildAndExpectSuccess(*builder); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); ASSERT_NE(metadata, nullptr); EXPECT_EQ(metadata->format_version, 2); EXPECT_EQ(metadata->last_sequence_number, TableMetadata::kInitialSequenceNumber); - EXPECT_EQ(metadata->default_spec_id, TableMetadata::kInitialSpecId); - EXPECT_EQ(metadata->default_sort_order_id, TableMetadata::kInitialSortOrderId); - EXPECT_EQ(metadata->current_snapshot_id, TableMetadata::kInvalidSnapshotId); + EXPECT_EQ(metadata->default_spec_id, PartitionSpec::kInitialSpecId); + EXPECT_EQ(metadata->default_sort_order_id, SortOrder::kInitialSortOrderId); + EXPECT_EQ(metadata->current_snapshot_id, Snapshot::kInvalidSnapshotId); } TEST_F(TableMetadataBuilderTest, BuildFromExisting) { auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); ASSERT_NE(builder, nullptr); - auto metadata = BuildAndExpectSuccess(*builder); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); ASSERT_NE(metadata, nullptr); EXPECT_EQ(metadata->format_version, 2); @@ -135,7 +111,7 @@ TEST_F(TableMetadataBuilderTest, AssignUUIDForNewTable) { auto builder = TableMetadataBuilder::BuildFromEmpty(2); builder->AssignUUID("new-uuid-5678"); - auto metadata = BuildAndExpectSuccess(*builder); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); EXPECT_EQ(metadata->table_uuid, "new-uuid-5678"); } @@ -143,7 +119,7 @@ TEST_F(TableMetadataBuilderTest, AssignUUIDAndUpdateExisting) { auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); builder->AssignUUID("updated-uuid-9999"); - auto metadata = BuildAndExpectSuccess(*builder); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); EXPECT_EQ(metadata->table_uuid, "updated-uuid-9999"); } @@ -151,14 +127,14 @@ TEST_F(TableMetadataBuilderTest, AssignUUIDWithEmptyUUID) { auto builder = TableMetadataBuilder::BuildFromEmpty(2); builder->AssignUUID(""); - BuildAndExpectFailure(*builder, "Cannot assign null or empty UUID"); + ASSERT_THAT(builder->Build(), HasErrorMessage("Cannot assign empty UUID")); } TEST_F(TableMetadataBuilderTest, AssignUUIDWithSameUUID) { auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); builder->AssignUUID("test-uuid-1234"); // Same UUID - auto metadata = BuildAndExpectSuccess(*builder); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); EXPECT_EQ(metadata->table_uuid, "test-uuid-1234"); } @@ -166,7 +142,7 @@ TEST_F(TableMetadataBuilderTest, AssignUUIDWithAutoGenerate) { auto builder = TableMetadataBuilder::BuildFromEmpty(2); builder->AssignUUID(); // Auto-generate - auto metadata = BuildAndExpectSuccess(*builder); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); EXPECT_FALSE(metadata->table_uuid.empty()); } @@ -175,23 +151,10 @@ TEST_F(TableMetadataBuilderTest, AssignUUIDAndCaseInsensitiveComparison) { auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); builder->AssignUUID("test-uuid-abcd"); // Different case - should be no-op - auto metadata = BuildAndExpectSuccess(*builder); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); EXPECT_EQ(metadata->table_uuid, "TEST-UUID-ABCD"); // Original case preserved } -// ============================================================================ -// TableMetadataBuilder - DiscardChanges Tests -// ============================================================================ - -TEST_F(TableMetadataBuilderTest, DiscardChangesAndResetsToBase) { - auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); - builder->AssignUUID("new-uuid"); - builder->DiscardChanges(); - - auto metadata = BuildAndExpectSuccess(*builder); - EXPECT_EQ(metadata->table_uuid, "test-uuid-1234"); // Original UUID -} - // ============================================================================ // TableUpdate - ApplyTo Tests // ============================================================================ @@ -202,7 +165,7 @@ TEST_F(TableMetadataBuilderTest, TableUpdateWithAssignUUID) { table::AssignUUID update("apply-uuid"); update.ApplyTo(*builder); - auto metadata = BuildAndExpectSuccess(*builder); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); EXPECT_EQ(metadata->table_uuid, "apply-uuid"); } @@ -242,39 +205,30 @@ TEST_F(TableMetadataBuilderTest, TEST_F(TableMetadataBuilderTest, TableRequirementAssertUUIDSuccess) { table::AssertUUID requirement("test-uuid-1234"); - auto status = requirement.Validate(base_metadata_.get()); - - EXPECT_TRUE(status.has_value()) << "Validation failed: " << status.error().message; + ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk()); } TEST_F(TableMetadataBuilderTest, TableRequirementAssertUUIDMismatch) { table::AssertUUID requirement("wrong-uuid"); auto status = requirement.Validate(base_metadata_.get()); - - ASSERT_FALSE(status.has_value()); - EXPECT_EQ(status.error().kind, ErrorKind::kCommitFailed); - EXPECT_TRUE(status.error().message.find("UUID does not match") != std::string::npos); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("UUID does not match")); } TEST_F(TableMetadataBuilderTest, TableRequirementAssertUUIDNullBase) { table::AssertUUID requirement("any-uuid"); auto status = requirement.Validate(nullptr); - - ASSERT_FALSE(status.has_value()); - EXPECT_EQ(status.error().kind, ErrorKind::kCommitFailed); - EXPECT_TRUE(status.error().message.find("metadata is missing") != std::string::npos); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("metadata is missing")); } TEST_F(TableMetadataBuilderTest, TableRequirementAssertUUIDCaseInsensitive) { base_metadata_->table_uuid = "TEST-UUID-1234"; table::AssertUUID requirement("test-uuid-1234"); - auto status = requirement.Validate(base_metadata_.get()); - - EXPECT_TRUE(status.has_value()) - << "Validation should succeed with case-insensitive match"; + ASSERT_THAT(requirement.Validate(base_metadata_.get()), IsOk()); } // ============================================================================ @@ -285,7 +239,7 @@ TEST_F(TableMetadataBuilderTest, IntegrationCreateTableWithUUID) { auto builder = TableMetadataBuilder::BuildFromEmpty(2); builder->AssignUUID("integration-test-uuid"); - auto metadata = BuildAndExpectSuccess(*builder); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); EXPECT_EQ(metadata->table_uuid, "integration-test-uuid"); EXPECT_EQ(metadata->format_version, 2); } @@ -304,7 +258,7 @@ TEST_F(TableMetadataBuilderTest, IntegrationOptimisticConcurrencyControl) { auto builder = TableMetadataBuilder::BuildFrom(base_metadata_.get()); update.ApplyTo(*builder); - auto metadata = BuildAndExpectSuccess(*builder); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build()); ASSERT_NE(metadata, nullptr); } diff --git a/src/iceberg/util/string_util.h b/src/iceberg/util/string_util.h index a22aa7a5d..aafb740cd 100644 --- a/src/iceberg/util/string_util.h +++ b/src/iceberg/util/string_util.h @@ -45,7 +45,7 @@ class ICEBERG_EXPORT StringUtils { return input; } - static bool EqualsIgnoreCase(const std::string& lhs, const std::string& rhs) { + static bool EqualsIgnoreCase(std::string_view lhs, std::string_view rhs) { return std::ranges::equal( lhs, rhs, [](char lc, char rc) { return std::tolower(lc) == std::tolower(rc); }); } From 0203492975f1e6b26fb99160aba222a0b4a2bdeb Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Fri, 24 Oct 2025 13:06:39 +0800 Subject: [PATCH 3/5] fix build error --- src/iceberg/test/meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index 3496c6a14..89cb3579b 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -47,9 +47,9 @@ iceberg_tests = { 'sources': files( 'json_internal_test.cc', 'schema_json_test.cc', + 'table_metadata_builder_test.cc', 'table_test.cc', 'test_common.cc', - 'table_metadata_builder_test.cc', ), }, 'expression_test': { From 90cba7c568a1c85b0875dd2e77be2f3c9bd8c021 Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Mon, 27 Oct 2025 10:18:10 +0800 Subject: [PATCH 4/5] resole review comments --- src/iceberg/table_metadata.cc | 4 ++-- src/iceberg/table_metadata.h | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index 04475eebe..2a3e2a026 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -224,7 +224,7 @@ struct TableMetadataBuilder::Impl { explicit Impl(int8_t format_version) : base(nullptr), metadata{} { metadata.format_version = format_version; metadata.last_sequence_number = TableMetadata::kInitialSequenceNumber; - metadata.last_updated_ms = TimePointMs::min(); + metadata.last_updated_ms = TableMetadata::kInvalidLastUpdatedMs; metadata.last_column_id = Schema::kInvalidColumnId; metadata.default_spec_id = PartitionSpec::kInitialSpecId; metadata.last_partition_id = PartitionSpec::kInvalidPartitionFieldId; @@ -448,7 +448,7 @@ Result> TableMetadataBuilder::Build() { // 2. Validate metadata consistency through TableMetadata#Validate // 3. Update last_updated_ms if there are changes - if (impl_->metadata.last_updated_ms == TimePointMs::min()) { + if (impl_->metadata.last_updated_ms == TableMetadata::kInvalidLastUpdatedMs) { impl_->metadata.last_updated_ms = TimePointMs{std::chrono::duration_cast( std::chrono::system_clock::now().time_since_epoch())}; diff --git a/src/iceberg/table_metadata.h b/src/iceberg/table_metadata.h index 2a998c7a1..c806fde90 100644 --- a/src/iceberg/table_metadata.h +++ b/src/iceberg/table_metadata.h @@ -73,6 +73,7 @@ struct ICEBERG_EXPORT TableMetadata { static constexpr int64_t kInitialSequenceNumber = 0; static constexpr int64_t kInvalidSequenceNumber = -1; static constexpr int64_t kInitialRowId = 0; + static constexpr TimePointMs kInvalidLastUpdatedMs = TimePointMs::min(); /// An integer version number for the format int8_t format_version; From f76c55b3406143d84c3aba10c77aae49466261a8 Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Mon, 27 Oct 2025 10:28:27 +0800 Subject: [PATCH 5/5] resolve review comments --- src/iceberg/table_metadata.cc | 8 ++++++-- src/iceberg/table_metadata.h | 1 - 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index 2a3e2a026..669e5a15b 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -41,6 +41,10 @@ namespace iceberg { +namespace { +const TimePointMs kInvalidLastUpdatedMs = TimePointMs::min(); +} + std::string ToString(const SnapshotLogEntry& entry) { return std::format("SnapshotLogEntry[timestampMillis={},snapshotId={}]", entry.timestamp_ms, entry.snapshot_id); @@ -224,7 +228,7 @@ struct TableMetadataBuilder::Impl { explicit Impl(int8_t format_version) : base(nullptr), metadata{} { metadata.format_version = format_version; metadata.last_sequence_number = TableMetadata::kInitialSequenceNumber; - metadata.last_updated_ms = TableMetadata::kInvalidLastUpdatedMs; + metadata.last_updated_ms = kInvalidLastUpdatedMs; metadata.last_column_id = Schema::kInvalidColumnId; metadata.default_spec_id = PartitionSpec::kInitialSpecId; metadata.last_partition_id = PartitionSpec::kInvalidPartitionFieldId; @@ -448,7 +452,7 @@ Result> TableMetadataBuilder::Build() { // 2. Validate metadata consistency through TableMetadata#Validate // 3. Update last_updated_ms if there are changes - if (impl_->metadata.last_updated_ms == TableMetadata::kInvalidLastUpdatedMs) { + if (impl_->metadata.last_updated_ms == kInvalidLastUpdatedMs) { impl_->metadata.last_updated_ms = TimePointMs{std::chrono::duration_cast( std::chrono::system_clock::now().time_since_epoch())}; diff --git a/src/iceberg/table_metadata.h b/src/iceberg/table_metadata.h index c806fde90..2a998c7a1 100644 --- a/src/iceberg/table_metadata.h +++ b/src/iceberg/table_metadata.h @@ -73,7 +73,6 @@ struct ICEBERG_EXPORT TableMetadata { static constexpr int64_t kInitialSequenceNumber = 0; static constexpr int64_t kInvalidSequenceNumber = -1; static constexpr int64_t kInitialRowId = 0; - static constexpr TimePointMs kInvalidLastUpdatedMs = TimePointMs::min(); /// An integer version number for the format int8_t format_version;