From 47680bd1e723e72088ac6eef7af2313e9ecd1da5 Mon Sep 17 00:00:00 2001 From: Zhao Junwang Date: Mon, 2 Nov 2026 00:55:20 +0800 Subject: [PATCH 1/2] fix(catalog): honor format-version when creating tables SqlCatalog::CreateTable previously always created V2 tables and rejected an explicit format-version table property as a reserved key, preventing callers from selecting the table format version during creation. Resolve the requested version in the shared TableMetadata::Make entry point, validate it before narrowing, and remove the control property before persisting table properties. Keep V2 as the default and preserve the explicit-version overload's reserved-property checks. Preserve the initial version through staged create transactions. Initialize SQL and in-memory catalog create builders from the first version update and replay subsequent upgrades in order, allowing staged tables to upgrade before their create transaction commits. Add parameterized coverage for supported and invalid versions, reserved properties, ordinary and staged creates, and staged V1/V2-to-V3 upgrades. Verify the version and properties after reload and in SQL metadata JSON. --- .../catalog/memory/in_memory_catalog.cc | 2 + src/iceberg/catalog/sql/sql_catalog.cc | 2 + src/iceberg/table_metadata.cc | 27 +++- src/iceberg/table_metadata.h | 14 +- src/iceberg/test/in_memory_catalog_test.cc | 110 ++++++++++++++ src/iceberg/test/sql_catalog_test.cc | 126 ++++++++++++++++ .../test/table_metadata_builder_test.cc | 138 ++++++++++++++++++ src/iceberg/transaction.cc | 3 +- 8 files changed, 418 insertions(+), 4 deletions(-) diff --git a/src/iceberg/catalog/memory/in_memory_catalog.cc b/src/iceberg/catalog/memory/in_memory_catalog.cc index 2ca198335..e7229bef3 100644 --- a/src/iceberg/catalog/memory/in_memory_catalog.cc +++ b/src/iceberg/catalog/memory/in_memory_catalog.cc @@ -467,6 +467,8 @@ Result> InMemoryCatalog::UpdateTable( format_version = internal::checked_cast(*update) .format_version(); + // Start at the initial version and replay later upgrades in order. + break; } } builder = TableMetadataBuilder::BuildFromEmpty(format_version); diff --git a/src/iceberg/catalog/sql/sql_catalog.cc b/src/iceberg/catalog/sql/sql_catalog.cc index e69729327..169612f49 100644 --- a/src/iceberg/catalog/sql/sql_catalog.cc +++ b/src/iceberg/catalog/sql/sql_catalog.cc @@ -449,6 +449,8 @@ Result> SqlCatalog::UpdateTable( format_version = iceberg::internal::checked_cast(*update) .format_version(); + // Start at the initial version and replay later upgrades in order. + break; } } builder = TableMetadataBuilder::BuildFromEmpty(format_version); diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index b67ab32fe..47cd607d3 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -54,6 +54,7 @@ #include "iceberg/util/location_util.h" #include "iceberg/util/macros.h" #include "iceberg/util/property_util.h" +#include "iceberg/util/string_util.h" #include "iceberg/util/timepoint.h" #include "iceberg/util/type_util.h" #include "iceberg/util/uuid.h" @@ -194,10 +195,32 @@ std::string ToString(const MetadataLogEntry& entry) { entry.metadata_file); } +Result> TableMetadata::Make( + const iceberg::Schema& schema, const iceberg::PartitionSpec& spec, + const iceberg::SortOrder& sort_order, const std::string& location, + const std::unordered_map& properties) { + auto persisted_properties = properties; + int format_version = kDefaultTableFormatVersion; + auto iter = persisted_properties.find(TableProperties::kFormatVersion.key()); + if (iter != persisted_properties.end()) { + auto parsed_version = StringUtils::ParseNumber(iter->second); + if (!parsed_version.has_value()) { + return InvalidArgument("Invalid format-version property: '{}'", iter->second); + } + format_version = parsed_version.value(); + persisted_properties.erase(iter); + } + return Make(schema, spec, sort_order, location, persisted_properties, format_version); +} + Result> TableMetadata::Make( const iceberg::Schema& schema, const iceberg::PartitionSpec& spec, const iceberg::SortOrder& sort_order, const std::string& location, const std::unordered_map& properties, int format_version) { + if (format_version < 1 || format_version > kSupportedTableFormatVersion) { + return InvalidArgument("Unsupported table format version: {} (supported: 1-{})", + format_version, kSupportedTableFormatVersion); + } for (const auto& [key, _] : properties) { if (TableProperties::reserved_properties().contains(key)) { return InvalidArgument( @@ -227,7 +250,7 @@ Result> TableMetadata::Make( ICEBERG_RETURN_UNEXPECTED(PropertyUtil::ValidateCommitProperties(properties)); - return TableMetadataBuilder::BuildFromEmpty(format_version) + return TableMetadataBuilder::BuildFromEmpty(static_cast(format_version)) ->AssignUUID() .SetLocation(location) .SetCurrentSchema(std::move(fresh_schema), last_column_id) @@ -567,6 +590,8 @@ class TableMetadataBuilder::Impl { // Constructor for new table explicit Impl(int8_t format_version) : base_(nullptr), metadata_{} { metadata_.format_version = format_version; + // Create commits must carry the initial version even when no upgrade occurs. + changes_.push_back(std::make_unique(format_version)); metadata_.last_sequence_number = TableMetadata::kInitialSequenceNumber; metadata_.last_updated_ms = kInvalidLastUpdatedMs; metadata_.last_column_id = Schema::kInvalidColumnId; diff --git a/src/iceberg/table_metadata.h b/src/iceberg/table_metadata.h index 6e2d9fe26..d15bd542d 100644 --- a/src/iceberg/table_metadata.h +++ b/src/iceberg/table_metadata.h @@ -132,11 +132,21 @@ struct ICEBERG_EXPORT TableMetadata { /// A list of encrypted table key entries std::vector encryption_keys; + /// \brief Create table metadata from user-supplied properties. + /// \note The reserved format-version property selects the table format version, + /// defaulting to kDefaultTableFormatVersion, and is not persisted. Other reserved + /// properties are rejected. The input properties are not modified. static Result> Make( const iceberg::Schema& schema, const iceberg::PartitionSpec& spec, const iceberg::SortOrder& sort_order, const std::string& location, - const std::unordered_map& properties, - int format_version = kDefaultTableFormatVersion); + const std::unordered_map& properties); + + /// \brief Create table metadata with an explicit supported format version. + /// \note All reserved properties, including format-version, are rejected. + static Result> Make( + const iceberg::Schema& schema, const iceberg::PartitionSpec& spec, + const iceberg::SortOrder& sort_order, const std::string& location, + const std::unordered_map& properties, int format_version); /// \brief Get the current schema, return NotFoundError if not found /// \note The returned schema is guaranteed to be not null diff --git a/src/iceberg/test/in_memory_catalog_test.cc b/src/iceberg/test/in_memory_catalog_test.cc index db90aa14a..06410bcec 100644 --- a/src/iceberg/test/in_memory_catalog_test.cc +++ b/src/iceberg/test/in_memory_catalog_test.cc @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -140,6 +141,115 @@ TEST_F(InMemoryCatalogTest, CreateTable) { EXPECT_THAT(table2, IsError(ErrorKind::kAlreadyExists)); } +class InMemoryCatalogFormatVersionTest + : public InMemoryCatalogTest, + public ::testing::WithParamInterface> {}; + +TEST_P(InMemoryCatalogFormatVersionTest, CreateTable) { + auto schema = std::make_shared( + std::vector{SchemaField::MakeRequired(1, "x", int64())}); + const auto [staged, version] = GetParam(); + TableIdentifier ident{.ns = {}, .name = "versioned_table"}; + const auto location = GenerateTestTableLocation(ident.name); + const std::unordered_map properties = { + {"format-version", std::to_string(version)}, {"owner", "alice"}}; + std::shared_ptr created; + if (staged) { + ICEBERG_UNWRAP_OR_FAIL( + auto transaction, + catalog_->StageCreateTable(ident, schema, PartitionSpec::Unpartitioned(), + SortOrder::Unsorted(), location, properties)); + EXPECT_EQ(transaction->current().format_version, version); + EXPECT_THAT(catalog_->TableExists(ident), HasValue(::testing::Eq(false))); + ICEBERG_UNWRAP_OR_FAIL(created, transaction->Commit()); + } else { + ICEBERG_UNWRAP_OR_FAIL( + created, catalog_->CreateTable(ident, schema, PartitionSpec::Unpartitioned(), + SortOrder::Unsorted(), location, properties)); + } + EXPECT_EQ(created->metadata()->format_version, version); + ICEBERG_UNWRAP_OR_FAIL(auto loaded, catalog_->LoadTable(ident)); + EXPECT_EQ(loaded->metadata()->format_version, version); + EXPECT_THAT(loaded->properties().configs(), + ::testing::UnorderedElementsAre(::testing::Pair("owner", "alice"))); +} + +INSTANTIATE_TEST_SUITE_P(SupportedVersions, InMemoryCatalogFormatVersionTest, + ::testing::Combine(::testing::Bool(), + ::testing::Values(1, 2, 3)), + [](const ::testing::TestParamInfo>& info) { + return std::string(std::get<0>(info.param) ? "StagedV" + : "CreateV") + + std::to_string(std::get<1>(info.param)); + }); + +class InMemoryCatalogStagedFormatUpgradeTest : public InMemoryCatalogTest, + public ::testing::WithParamInterface { +}; + +TEST_P(InMemoryCatalogStagedFormatUpgradeTest, UpgradeBeforeCreateCommit) { + auto schema = std::make_shared( + std::vector{SchemaField::MakeRequired(1, "x", int64())}); + TableIdentifier ident{.ns = {}, .name = "upgraded_table"}; + ICEBERG_UNWRAP_OR_FAIL(auto transaction, + catalog_->StageCreateTable( + ident, schema, PartitionSpec::Unpartitioned(), + SortOrder::Unsorted(), GenerateTestTableLocation(ident.name), + {{"format-version", std::to_string(GetParam())}})); + ICEBERG_UNWRAP_OR_FAIL(auto update, transaction->NewUpdateProperties()); + ASSERT_THAT(update->Set("format-version", "3").Set("owner", "alice").Commit(), IsOk()); + EXPECT_EQ(transaction->current().format_version, 3); + EXPECT_THAT(catalog_->TableExists(ident), HasValue(::testing::Eq(false))); + + ICEBERG_UNWRAP_OR_FAIL(auto created, transaction->Commit()); + EXPECT_EQ(created->metadata()->format_version, 3); + ICEBERG_UNWRAP_OR_FAIL(auto loaded, catalog_->LoadTable(ident)); + EXPECT_EQ(loaded->metadata()->format_version, 3); + EXPECT_THAT(loaded->properties().configs(), + ::testing::UnorderedElementsAre(::testing::Pair("owner", "alice"))); +} + +INSTANTIATE_TEST_SUITE_P(StagedUpgrades, InMemoryCatalogStagedFormatUpgradeTest, + ::testing::Values(1, 2), + [](const ::testing::TestParamInfo& info) { + return "V" + std::to_string(info.param) + "ToV3"; + }); + +class InMemoryCatalogInvalidFormatVersionTest + : public InMemoryCatalogTest, + public ::testing::WithParamInterface> {}; + +TEST_P(InMemoryCatalogInvalidFormatVersionTest, DoesNotCreateTableOrMetadata) { + auto schema = std::make_shared( + std::vector{SchemaField::MakeRequired(1, "x", int64())}); + const auto& [staged, version] = GetParam(); + TableIdentifier ident{.ns = {}, .name = "invalid_version"}; + const auto location = GenerateTestTableLocation(ident.name); + const std::unordered_map properties = { + {"format-version", version}}; + if (staged) { + EXPECT_THAT(catalog_->StageCreateTable(ident, schema, PartitionSpec::Unpartitioned(), + SortOrder::Unsorted(), location, properties), + IsError(ErrorKind::kInvalidArgument)); + } else { + EXPECT_THAT(catalog_->CreateTable(ident, schema, PartitionSpec::Unpartitioned(), + SortOrder::Unsorted(), location, properties), + IsError(ErrorKind::kInvalidArgument)); + } + EXPECT_THAT(catalog_->TableExists(ident), HasValue(::testing::Eq(false))); + EXPECT_TRUE(std::filesystem::is_empty(std::filesystem::path(location) / "metadata")); +} + +INSTANTIATE_TEST_SUITE_P( + InvalidVersions, InMemoryCatalogInvalidFormatVersionTest, + ::testing::Combine(::testing::Bool(), + ::testing::Values("", "abc", "2x", "1.5", "2147483648", "0", "-1", + "4", "257")), + [](const ::testing::TestParamInfo>& info) { + return std::string(std::get<0>(info.param) ? "StagedCase" : "CreateCase") + + std::to_string(info.index); + }); + TEST_F(InMemoryCatalogTest, RegisterTable) { TableIdentifier tableIdent{.ns = {}, .name = "t1"}; diff --git a/src/iceberg/test/sql_catalog_test.cc b/src/iceberg/test/sql_catalog_test.cc index c6531eeae..f46877c35 100644 --- a/src/iceberg/test/sql_catalog_test.cc +++ b/src/iceberg/test/sql_catalog_test.cc @@ -22,15 +22,18 @@ #include #include +#include #include #include #include #include +#include #include #include #include #include +#include #include "iceberg/arrow/arrow_io_internal.h" #include "iceberg/catalog/sql/catalog_store.h" @@ -41,8 +44,11 @@ #include "iceberg/sort_order.h" #include "iceberg/table.h" #include "iceberg/table_identifier.h" +#include "iceberg/table_metadata.h" #include "iceberg/test/matchers.h" +#include "iceberg/transaction.h" #include "iceberg/type.h" +#include "iceberg/update/update_properties.h" namespace iceberg::sql { @@ -291,6 +297,126 @@ TEST_F(SqlCatalogTest, TableLifecycle) { EXPECT_THAT(catalog_->DropTable(renamed, false), IsError(ErrorKind::kNoSuchTable)); } +class SqlCatalogFormatVersionTest + : public SqlCatalogTest, + public ::testing::WithParamInterface> {}; + +TEST_P(SqlCatalogFormatVersionTest, CreateTable) { + Namespace ns{{"db"}}; + ASSERT_THAT(catalog_->CreateNamespace(ns, {}), IsOk()); + const auto [staged, version] = GetParam(); + TableIdentifier ident{.ns = ns, .name = "versioned_table"}; + const auto location = MakeTableLocation(ident.name); + const std::unordered_map properties = { + {"format-version", std::to_string(version)}, {"owner", "alice"}}; + std::shared_ptr
created; + if (staged) { + ICEBERG_UNWRAP_OR_FAIL( + auto transaction, + catalog_->StageCreateTable(ident, MakeSchema(), PartitionSpec::Unpartitioned(), + SortOrder::Unsorted(), location, properties)); + EXPECT_EQ(transaction->current().format_version, version); + EXPECT_THAT(catalog_->TableExists(ident), HasValue(::testing::Eq(false))); + ICEBERG_UNWRAP_OR_FAIL(created, transaction->Commit()); + } else { + ICEBERG_UNWRAP_OR_FAIL( + created, + catalog_->CreateTable(ident, MakeSchema(), PartitionSpec::Unpartitioned(), + SortOrder::Unsorted(), location, properties)); + } + EXPECT_EQ(created->metadata()->format_version, version); + ICEBERG_UNWRAP_OR_FAIL(auto loaded, catalog_->LoadTable(ident)); + EXPECT_EQ(loaded->metadata()->format_version, version); + EXPECT_THAT(loaded->properties().configs(), + ::testing::UnorderedElementsAre(::testing::Pair("owner", "alice"))); + std::ifstream input(std::string(loaded->metadata_file_location())); + ASSERT_TRUE(input.is_open()); + const auto json = nlohmann::json::parse(input); + EXPECT_EQ(json.at("format-version"), version); + EXPECT_FALSE(json.at("properties").contains("format-version")); +} + +INSTANTIATE_TEST_SUITE_P(SupportedVersions, SqlCatalogFormatVersionTest, + ::testing::Combine(::testing::Bool(), + ::testing::Values(1, 2, 3)), + [](const ::testing::TestParamInfo>& info) { + return std::string(std::get<0>(info.param) ? "StagedV" + : "CreateV") + + std::to_string(std::get<1>(info.param)); + }); + +class SqlCatalogStagedFormatUpgradeTest : public SqlCatalogTest, + public ::testing::WithParamInterface {}; + +TEST_P(SqlCatalogStagedFormatUpgradeTest, UpgradeBeforeCreateCommit) { + Namespace ns{{"db"}}; + ASSERT_THAT(catalog_->CreateNamespace(ns, {}), IsOk()); + TableIdentifier ident{.ns = ns, .name = "upgraded_table"}; + ICEBERG_UNWRAP_OR_FAIL( + auto transaction, + catalog_->StageCreateTable(ident, MakeSchema(), PartitionSpec::Unpartitioned(), + SortOrder::Unsorted(), MakeTableLocation(ident.name), + {{"format-version", std::to_string(GetParam())}})); + ICEBERG_UNWRAP_OR_FAIL(auto update, transaction->NewUpdateProperties()); + ASSERT_THAT(update->Set("format-version", "3").Set("owner", "alice").Commit(), IsOk()); + EXPECT_EQ(transaction->current().format_version, 3); + EXPECT_THAT(catalog_->TableExists(ident), HasValue(::testing::Eq(false))); + + ICEBERG_UNWRAP_OR_FAIL(auto created, transaction->Commit()); + EXPECT_EQ(created->metadata()->format_version, 3); + ICEBERG_UNWRAP_OR_FAIL(auto loaded, catalog_->LoadTable(ident)); + EXPECT_EQ(loaded->metadata()->format_version, 3); + EXPECT_THAT(loaded->properties().configs(), + ::testing::UnorderedElementsAre(::testing::Pair("owner", "alice"))); + std::ifstream input(std::string(loaded->metadata_file_location())); + ASSERT_TRUE(input.is_open()); + const auto json = nlohmann::json::parse(input); + EXPECT_EQ(json.at("format-version"), 3); + EXPECT_FALSE(json.at("properties").contains("format-version")); +} + +INSTANTIATE_TEST_SUITE_P(StagedUpgrades, SqlCatalogStagedFormatUpgradeTest, + ::testing::Values(1, 2), + [](const ::testing::TestParamInfo& info) { + return "V" + std::to_string(info.param) + "ToV3"; + }); + +class SqlCatalogInvalidFormatVersionTest + : public SqlCatalogTest, + public ::testing::WithParamInterface> {}; + +TEST_P(SqlCatalogInvalidFormatVersionTest, DoesNotCreateTableOrMetadata) { + Namespace ns{{"db"}}; + ASSERT_THAT(catalog_->CreateNamespace(ns, {}), IsOk()); + const auto& [staged, version] = GetParam(); + TableIdentifier ident{.ns = ns, .name = "invalid_version"}; + const auto location = MakeTableLocation(ident.name); + const std::unordered_map properties = { + {"format-version", version}}; + if (staged) { + EXPECT_THAT( + catalog_->StageCreateTable(ident, MakeSchema(), PartitionSpec::Unpartitioned(), + SortOrder::Unsorted(), location, properties), + IsError(ErrorKind::kInvalidArgument)); + } else { + EXPECT_THAT(catalog_->CreateTable(ident, MakeSchema(), PartitionSpec::Unpartitioned(), + SortOrder::Unsorted(), location, properties), + IsError(ErrorKind::kInvalidArgument)); + } + EXPECT_THAT(catalog_->TableExists(ident), HasValue(::testing::Eq(false))); + EXPECT_TRUE(std::filesystem::is_empty(std::filesystem::path(location) / "metadata")); +} + +INSTANTIATE_TEST_SUITE_P( + InvalidVersions, SqlCatalogInvalidFormatVersionTest, + ::testing::Combine(::testing::Bool(), + ::testing::Values("", "abc", "2x", "1.5", "2147483648", "0", "-1", + "4", "257")), + [](const ::testing::TestParamInfo>& info) { + return std::string(std::get<0>(info.param) ? "StagedCase" : "CreateCase") + + std::to_string(info.index); + }); + TEST_F(SqlCatalogTest, CreateTableRequiresNamespace) { TableIdentifier ident{.ns = Namespace{{"missing"}}, .name = "t1"}; EXPECT_THAT(catalog_->CreateTable(ident, MakeSchema(), PartitionSpec::Unpartitioned(), diff --git a/src/iceberg/test/table_metadata_builder_test.cc b/src/iceberg/test/table_metadata_builder_test.cc index 5a9681add..dd61c9533 100644 --- a/src/iceberg/test/table_metadata_builder_test.cc +++ b/src/iceberg/test/table_metadata_builder_test.cc @@ -17,6 +17,7 @@ * under the License. */ +#include #include #include #include @@ -108,6 +109,7 @@ TEST(TableMetadataTest, Make) { ICEBERG_UNWRAP_OR_FAIL( auto metadata, TableMetadata::Make(*Schema, *spec, *order, "s3://bucket/test", {})); + EXPECT_EQ(metadata->format_version, TableMetadata::kDefaultTableFormatVersion); EXPECT_THAT(Uuid::FromString(metadata->table_uuid), IsOk()); // Check schema fields ASSERT_EQ(1, metadata->schemas.size()); @@ -143,6 +145,142 @@ TEST(TableMetadataTest, Make) { EXPECT_EQ(NullOrder::kLast, order_fields[0].null_order()); } +class TableMetadataFormatVersionTest : public ::testing::TestWithParam {}; + +TEST_P(TableMetadataFormatVersionTest, MakeWithProperty) { + auto schema = CreateTestSchema(); + auto spec = PartitionSpec::Unpartitioned(); + auto order = SortOrder::Unsorted(); + const int version = GetParam(); + const std::unordered_map properties = { + {"format-version", std::to_string(version)}, {"owner", "alice"}}; + ICEBERG_UNWRAP_OR_FAIL( + auto metadata, + TableMetadata::Make(*schema, *spec, *order, "s3://bucket/test", properties)); + EXPECT_EQ(metadata->format_version, version); + EXPECT_THAT(metadata->properties.configs(), + ::testing::UnorderedElementsAre(::testing::Pair("owner", "alice"))); + EXPECT_EQ(properties.at("format-version"), std::to_string(version)); +} + +TEST_P(TableMetadataFormatVersionTest, MakeWithExplicitVersion) { + auto schema = CreateTestSchema(); + auto spec = PartitionSpec::Unpartitioned(); + auto order = SortOrder::Unsorted(); + ICEBERG_UNWRAP_OR_FAIL( + auto metadata, + TableMetadata::Make(*schema, *spec, *order, "s3://bucket/test", {}, GetParam())); + EXPECT_EQ(metadata->format_version, GetParam()); +} + +INSTANTIATE_TEST_SUITE_P(SupportedVersions, TableMetadataFormatVersionTest, + ::testing::Values(1, 2, 3), + [](const ::testing::TestParamInfo& info) { + return "V" + std::to_string(info.param); + }); + +class TableMetadataInvalidFormatVersionTest : public ::testing::TestWithParam {}; + +TEST_P(TableMetadataInvalidFormatVersionTest, MakeRejectsProperty) { + auto schema = CreateTestSchema(); + auto spec = PartitionSpec::Unpartitioned(); + auto order = SortOrder::Unsorted(); + EXPECT_THAT(TableMetadata::Make(*schema, *spec, *order, "s3://bucket/test", + {{"format-version", std::to_string(GetParam())}}), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST_P(TableMetadataInvalidFormatVersionTest, MakeRejectsExplicitVersion) { + auto schema = CreateTestSchema(); + auto spec = PartitionSpec::Unpartitioned(); + auto order = SortOrder::Unsorted(); + EXPECT_THAT( + TableMetadata::Make(*schema, *spec, *order, "s3://bucket/test", {}, GetParam()), + IsError(ErrorKind::kInvalidArgument)); +} + +INSTANTIATE_TEST_SUITE_P(UnsupportedVersions, TableMetadataInvalidFormatVersionTest, + ::testing::Values(0, -1, 4, 257)); + +struct InvalidFormatVersionString { + std::string name; + std::string value; +}; + +class TableMetadataInvalidFormatVersionStringTest + : public ::testing::TestWithParam {}; + +TEST_P(TableMetadataInvalidFormatVersionStringTest, MakeRejectsProperty) { + auto schema = CreateTestSchema(); + auto spec = PartitionSpec::Unpartitioned(); + auto order = SortOrder::Unsorted(); + auto result = TableMetadata::Make(*schema, *spec, *order, "s3://bucket/test", + {{"format-version", GetParam().value}}); + EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(result, HasErrorMessage("Invalid format-version property")); +} + +INSTANTIATE_TEST_SUITE_P( + InvalidStrings, TableMetadataInvalidFormatVersionStringTest, + ::testing::Values(InvalidFormatVersionString{"Empty", ""}, + InvalidFormatVersionString{"NonNumeric", "abc"}, + InvalidFormatVersionString{"TrailingCharacters", "2x"}, + InvalidFormatVersionString{"Fractional", "1.5"}, + InvalidFormatVersionString{"Overflow", "2147483648"}, + InvalidFormatVersionString{"LeadingPlus", "+2"}, + InvalidFormatVersionString{"LeadingWhitespace", " 2"}, + InvalidFormatVersionString{"TrailingWhitespace", "2 "}), + [](const ::testing::TestParamInfo& info) { + return info.param.name; + }); + +class TableMetadataReservedPropertyTest : public ::testing::TestWithParam {}; + +TEST_P(TableMetadataReservedPropertyTest, MakeWithProperty) { + auto schema = CreateTestSchema(); + auto spec = PartitionSpec::Unpartitioned(); + auto order = SortOrder::Unsorted(); + auto result = TableMetadata::Make(*schema, *spec, *order, "s3://bucket/test", + {{GetParam(), "1"}}); + if (GetParam() == TableProperties::kFormatVersion.key()) { + ASSERT_THAT(result, IsOk()); + EXPECT_EQ(result.value()->format_version, 1); + EXPECT_TRUE(result.value()->properties.configs().empty()); + } else { + EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); + } +} + +TEST_P(TableMetadataReservedPropertyTest, ExplicitVersionRejectsReservedProperty) { + auto schema = CreateTestSchema(); + auto spec = PartitionSpec::Unpartitioned(); + auto order = SortOrder::Unsorted(); + EXPECT_THAT(TableMetadata::Make(*schema, *spec, *order, "s3://bucket/test", + {{GetParam(), "1"}}, 2), + IsError(ErrorKind::kInvalidArgument)); +} + +INSTANTIATE_TEST_SUITE_P(ReservedProperties, TableMetadataReservedPropertyTest, + ::testing::ValuesIn(TableProperties::reserved_properties()), + [](const ::testing::TestParamInfo& info) { + auto name = info.param; + std::ranges::replace(name, '-', '_'); + return name; + }); + +TEST(TableMetadataTest, MakeValidatesSchemaUsingRequestedFormatVersion) { + Schema schema({SchemaField::MakeRequired(1, "ts", timestamp_ns())}); + auto spec = PartitionSpec::Unpartitioned(); + auto order = SortOrder::Unsorted(); + auto result = TableMetadata::Make(schema, *spec, *order, "s3://bucket/test", + {{"format-version", "2"}}); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("timestamp_ns is not supported until v3")); + EXPECT_THAT(TableMetadata::Make(schema, *spec, *order, "s3://bucket/test", + {{"format-version", "3"}}), + IsOk()); +} + TEST(TableMetadataTest, MakeWithInvalidPartitionSpec) { ICEBERG_UNWRAP_OR_FAIL(auto schema, CreateDisorderedSchema()); ICEBERG_UNWRAP_OR_FAIL( diff --git a/src/iceberg/transaction.cc b/src/iceberg/transaction.cc index 0eaa3c976..4ac631702 100644 --- a/src/iceberg/transaction.cc +++ b/src/iceberg/transaction.cc @@ -114,7 +114,8 @@ Result> TransactionContext::Make( ctx->kind = kind; ctx->table = std::move(table); if (kind == TransactionKind::kCreate) { - ctx->metadata_builder = TableMetadataBuilder::BuildFromEmpty(); + ctx->metadata_builder = + TableMetadataBuilder::BuildFromEmpty(ctx->table->metadata()->format_version); std::ignore = ctx->metadata_builder->ApplyChangesForCreate(*ctx->table->metadata()); } else { ctx->metadata_builder = TableMetadataBuilder::BuildFrom(ctx->table->metadata().get()); From 8bb9c55ac83f27c4600bfbab633350d2aa4b9522 Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Thu, 5 Nov 2026 19:29:44 +0800 Subject: [PATCH 2/2] refactor(metadata): make explicit format-version overload private Keep table creation through the public properties-based factory and make the explicit format-version overload an internal implementation detail. Remove direct tests of the private overload, retaining public entry point coverage for supported versions, invalid versions, and reserved properties. --- src/iceberg/table_metadata.h | 15 +++++----- .../test/table_metadata_builder_test.cc | 28 ------------------- 2 files changed, 8 insertions(+), 35 deletions(-) diff --git a/src/iceberg/table_metadata.h b/src/iceberg/table_metadata.h index d15bd542d..2779d413f 100644 --- a/src/iceberg/table_metadata.h +++ b/src/iceberg/table_metadata.h @@ -141,13 +141,6 @@ struct ICEBERG_EXPORT TableMetadata { const iceberg::SortOrder& sort_order, const std::string& location, const std::unordered_map& properties); - /// \brief Create table metadata with an explicit supported format version. - /// \note All reserved properties, including format-version, are rejected. - static Result> Make( - const iceberg::Schema& schema, const iceberg::PartitionSpec& spec, - const iceberg::SortOrder& sort_order, const std::string& location, - const std::unordered_map& properties, int format_version); - /// \brief Get the current schema, return NotFoundError if not found /// \note The returned schema is guaranteed to be not null Result> Schema() const; @@ -178,6 +171,14 @@ struct ICEBERG_EXPORT TableMetadata { ICEBERG_EXPORT friend bool operator==(const TableMetadata& lhs, const TableMetadata& rhs); + + private: + /// \brief Create table metadata with an explicit supported format version. + /// \note All reserved properties, including format-version, are rejected. + static Result> Make( + const iceberg::Schema& schema, const iceberg::PartitionSpec& spec, + const iceberg::SortOrder& sort_order, const std::string& location, + const std::unordered_map& properties, int format_version); }; // Cache for table metadata mappings to facilitate fast lookups. diff --git a/src/iceberg/test/table_metadata_builder_test.cc b/src/iceberg/test/table_metadata_builder_test.cc index dd61c9533..bb5a50a19 100644 --- a/src/iceberg/test/table_metadata_builder_test.cc +++ b/src/iceberg/test/table_metadata_builder_test.cc @@ -163,16 +163,6 @@ TEST_P(TableMetadataFormatVersionTest, MakeWithProperty) { EXPECT_EQ(properties.at("format-version"), std::to_string(version)); } -TEST_P(TableMetadataFormatVersionTest, MakeWithExplicitVersion) { - auto schema = CreateTestSchema(); - auto spec = PartitionSpec::Unpartitioned(); - auto order = SortOrder::Unsorted(); - ICEBERG_UNWRAP_OR_FAIL( - auto metadata, - TableMetadata::Make(*schema, *spec, *order, "s3://bucket/test", {}, GetParam())); - EXPECT_EQ(metadata->format_version, GetParam()); -} - INSTANTIATE_TEST_SUITE_P(SupportedVersions, TableMetadataFormatVersionTest, ::testing::Values(1, 2, 3), [](const ::testing::TestParamInfo& info) { @@ -190,15 +180,6 @@ TEST_P(TableMetadataInvalidFormatVersionTest, MakeRejectsProperty) { IsError(ErrorKind::kInvalidArgument)); } -TEST_P(TableMetadataInvalidFormatVersionTest, MakeRejectsExplicitVersion) { - auto schema = CreateTestSchema(); - auto spec = PartitionSpec::Unpartitioned(); - auto order = SortOrder::Unsorted(); - EXPECT_THAT( - TableMetadata::Make(*schema, *spec, *order, "s3://bucket/test", {}, GetParam()), - IsError(ErrorKind::kInvalidArgument)); -} - INSTANTIATE_TEST_SUITE_P(UnsupportedVersions, TableMetadataInvalidFormatVersionTest, ::testing::Values(0, -1, 4, 257)); @@ -251,15 +232,6 @@ TEST_P(TableMetadataReservedPropertyTest, MakeWithProperty) { } } -TEST_P(TableMetadataReservedPropertyTest, ExplicitVersionRejectsReservedProperty) { - auto schema = CreateTestSchema(); - auto spec = PartitionSpec::Unpartitioned(); - auto order = SortOrder::Unsorted(); - EXPECT_THAT(TableMetadata::Make(*schema, *spec, *order, "s3://bucket/test", - {{GetParam(), "1"}}, 2), - IsError(ErrorKind::kInvalidArgument)); -} - INSTANTIATE_TEST_SUITE_P(ReservedProperties, TableMetadataReservedPropertyTest, ::testing::ValuesIn(TableProperties::reserved_properties()), [](const ::testing::TestParamInfo& info) {