From 47680bd1e723e72088ac6eef7af2313e9ecd1da5 Mon Sep 17 00:00:00 2001 From: Zhao Junwang Date: Mon, 2 Nov 2026 00:55:20 +0800 Subject: [PATCH] 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 2ca1983353..e7229bef30 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 e697293278..169612f49b 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 b67ab32fef..47cd607d3a 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 6e2d9fe268..d15bd542dc 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 db90aa14a9..06410bceca 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 c6531eeae2..f46877c357 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 5a9681addb..dd61c9533c 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 0eaa3c9762..4ac6317024 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());