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());