From 8a10475f4c08b52c884bfd62df8b5fa753c99c31 Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Fri, 30 Jan 2026 17:15:12 +0800 Subject: [PATCH 1/3] chore: remove MakeVxWriter functions in favor of MakeWriter Remove MakeVxWriter functions in favor of MakeWriter, add constants for iceberg table format versions, and apply int8_t to all format version parameters for consistency. --- src/iceberg/constants.h | 5 + src/iceberg/json_serde.cc | 28 ++- src/iceberg/manifest/manifest_writer.cc | 222 +++++------------- src/iceberg/manifest/manifest_writer.h | 77 ------ src/iceberg/table_metadata.cc | 14 +- src/iceberg/test/delete_file_index_test.cc | 58 ++--- src/iceberg/test/manifest_group_test.cc | 25 +- .../test/manifest_list_versions_test.cc | 30 +-- .../test/manifest_reader_stats_test.cc | 17 +- src/iceberg/test/manifest_reader_test.cc | 27 ++- .../test/manifest_writer_versions_test.cc | 63 ++--- .../test/rolling_manifest_writer_test.cc | 21 +- src/iceberg/test/table_scan_test.cc | 33 +-- src/iceberg/update/snapshot_update.cc | 4 +- src/iceberg/update/update_partition_spec.cc | 8 +- 15 files changed, 236 insertions(+), 396 deletions(-) diff --git a/src/iceberg/constants.h b/src/iceberg/constants.h index 1d5941626..8c81ab60c 100644 --- a/src/iceberg/constants.h +++ b/src/iceberg/constants.h @@ -30,6 +30,11 @@ namespace iceberg { +/// \brief Iceberg table format versions +constexpr int8_t kFormatVersion1 = 1; +constexpr int8_t kFormatVersion2 = 2; +constexpr int8_t kFormatVersion3 = 3; + constexpr std::string_view kParquetFieldIdKey = "PARQUET:field_id"; constexpr int64_t kInvalidSnapshotId = -1; constexpr int64_t kInvalidSequenceNumber = -1; diff --git a/src/iceberg/json_serde.cc b/src/iceberg/json_serde.cc index 93705f538..e920f4e3a 100644 --- a/src/iceberg/json_serde.cc +++ b/src/iceberg/json_serde.cc @@ -26,6 +26,7 @@ #include +#include "iceberg/constants.h" #include "iceberg/json_serde_internal.h" #include "iceberg/name_mapping.h" #include "iceberg/partition_field.h" @@ -837,7 +838,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) { json[kFormatVersion] = table_metadata.format_version; json[kTableUuid] = table_metadata.table_uuid; json[kLocation] = table_metadata.location; - if (table_metadata.format_version > 1) { + if (table_metadata.format_version > kFormatVersion1) { json[kLastSequenceNumber] = table_metadata.last_sequence_number; } json[kLastUpdatedMs] = UnixMsFromTimePointMs(table_metadata.last_updated_ms); @@ -846,7 +847,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) { // for older readers, continue writing the current schema as "schema". // this is only needed for v1 because support for schemas and current-schema-id // is required in v2 and later. - if (table_metadata.format_version == 1) { + if (table_metadata.format_version == kFormatVersion1) { for (const auto& schema : table_metadata.schemas) { if (schema->schema_id() == table_metadata.current_schema_id) { json[kSchema] = ToJson(*schema); @@ -860,7 +861,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) { json[kSchemas] = ToJsonList(table_metadata.schemas); // for older readers, continue writing the default spec as "partition-spec" - if (table_metadata.format_version == 1) { + if (table_metadata.format_version == kFormatVersion1) { for (const auto& partition_spec : table_metadata.partition_specs) { if (partition_spec->spec_id() == table_metadata.default_spec_id) { json[kPartitionSpec] = ToJson(*partition_spec); @@ -889,7 +890,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) { json[kCurrentSnapshotId] = nlohmann::json::value_t::null; } - if (table_metadata.format_version >= 3) { + if (table_metadata.format_version >= kFormatVersion3) { json[kNextRowId] = table_metadata.next_row_id; } @@ -944,7 +945,7 @@ Result> ParseSchemas( current_schema_id, SafeDumpJson(schema_array)); } } else { - if (format_version != 1) { + if (format_version != kFormatVersion1) { return JsonParseError("{} must exist in format v{}", kSchemas, format_version); } ICEBERG_ASSIGN_OR_RAISE(auto schema_json, @@ -982,7 +983,7 @@ Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version, partition_specs.push_back(std::move(spec)); } } else { - if (format_version != 1) { + if (format_version != kFormatVersion1) { return JsonParseError("{} must exist in format v{}", kPartitionSpecs, format_version); } @@ -998,8 +999,9 @@ Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version, std::vector fields; for (const auto& entry_json : partition_spec_json) { ICEBERG_ASSIGN_OR_RAISE( - auto field, PartitionFieldFromJson( - entry_json, /*allow_field_id_missing=*/format_version == 1)); + auto field, + PartitionFieldFromJson( + entry_json, /*allow_field_id_missing=*/format_version == kFormatVersion1)); int32_t field_id = field->field_id(); if (field_id == SchemaField::kInvalidFieldId) { // If the field ID is not set, we need to assign a new one @@ -1043,7 +1045,7 @@ Status ParseSortOrders(const nlohmann::json& json, int8_t format_version, sort_orders.push_back(std::move(sort_order)); } } else { - if (format_version > 1) { + if (format_version > kFormatVersion1) { return JsonParseError("{} must exist in format v{}", kSortOrders, format_version); } auto sort_order = SortOrder::Unsorted(); @@ -1065,7 +1067,7 @@ Result> TableMetadataFromJson(const nlohmann::jso ICEBERG_ASSIGN_OR_RAISE(table_metadata->format_version, GetJsonValue(json, kFormatVersion)); - if (table_metadata->format_version < 1 || + if (table_metadata->format_version < kFormatVersion1 || table_metadata->format_version > TableMetadata::kSupportedTableFormatVersion) { return JsonParseError("Cannot read unsupported version: {}", table_metadata->format_version); @@ -1076,7 +1078,7 @@ Result> TableMetadataFromJson(const nlohmann::jso ICEBERG_ASSIGN_OR_RAISE(table_metadata->location, GetJsonValue(json, kLocation)); - if (table_metadata->format_version > 1) { + if (table_metadata->format_version > kFormatVersion1) { ICEBERG_ASSIGN_OR_RAISE(table_metadata->last_sequence_number, GetJsonValue(json, kLastSequenceNumber)); } else { @@ -1098,7 +1100,7 @@ Result> TableMetadataFromJson(const nlohmann::jso ICEBERG_ASSIGN_OR_RAISE(table_metadata->last_partition_id, GetJsonValue(json, kLastPartitionId)); } else { - if (table_metadata->format_version > 1) { + if (table_metadata->format_version > kFormatVersion1) { return JsonParseError("{} must exist in format v{}", kLastPartitionId, table_metadata->format_version); } @@ -1128,7 +1130,7 @@ Result> TableMetadataFromJson(const nlohmann::jso table_metadata->current_snapshot_id, GetJsonValueOrDefault(json, kCurrentSnapshotId, kInvalidSnapshotId)); - if (table_metadata->format_version >= 3) { + if (table_metadata->format_version >= kFormatVersion3) { ICEBERG_ASSIGN_OR_RAISE(table_metadata->next_row_id, GetJsonValue(json, kNextRowId)); } else { diff --git a/src/iceberg/manifest/manifest_writer.cc b/src/iceberg/manifest/manifest_writer.cc index 36b34b8bc..55c6af128 100644 --- a/src/iceberg/manifest/manifest_writer.cc +++ b/src/iceberg/manifest/manifest_writer.cc @@ -30,7 +30,6 @@ #include "iceberg/partition_summary_internal.h" #include "iceberg/result.h" #include "iceberg/schema.h" -#include "iceberg/snapshot.h" #include "iceberg/table_metadata.h" #include "iceberg/util/macros.h" @@ -272,87 +271,41 @@ Result> OpenFileWriter( return writer; } -Result> ManifestWriter::MakeV1Writer( - std::optional snapshot_id, std::string_view manifest_location, - std::shared_ptr file_io, std::shared_ptr partition_spec, - std::shared_ptr current_schema) { - if (manifest_location.empty()) { - return InvalidArgument("Manifest location cannot be empty"); - } - if (!file_io) { - return InvalidArgument("FileIO cannot be null"); - } - if (!partition_spec) { - return InvalidArgument("PartitionSpec cannot be null"); - } - if (!current_schema) { - return InvalidArgument("Current schema cannot be null"); - } +Result> ManifestWriter::MakeWriter( + int8_t format_version, std::optional snapshot_id, + std::string_view manifest_location, std::shared_ptr file_io, + std::shared_ptr partition_spec, std::shared_ptr current_schema, + ManifestContent content, std::optional first_row_id) { + ICEBERG_PRECHECK(!manifest_location.empty(), "Manifest location cannot be empty"); + ICEBERG_PRECHECK(file_io, "FileIO cannot be null"); + ICEBERG_PRECHECK(partition_spec, "PartitionSpec cannot be null"); + ICEBERG_PRECHECK(current_schema, "Current schema cannot be null"); - auto adapter = std::make_unique( - snapshot_id, std::move(partition_spec), std::move(current_schema)); - ICEBERG_RETURN_UNEXPECTED(adapter->Init()); - ICEBERG_RETURN_UNEXPECTED(adapter->StartAppending()); + std::unique_ptr adapter; + std::optional writer_first_row_id = std::nullopt; - auto schema = adapter->schema(); - ICEBERG_ASSIGN_OR_RAISE( - auto writer, - OpenFileWriter(manifest_location, std::move(schema), std::move(file_io), - adapter->metadata(), "manifest_entry")); - return std::unique_ptr(new ManifestWriter( - std::move(writer), std::move(adapter), manifest_location, std::nullopt)); -} - -Result> ManifestWriter::MakeV2Writer( - std::optional snapshot_id, std::string_view manifest_location, - std::shared_ptr file_io, std::shared_ptr partition_spec, - std::shared_ptr current_schema, ManifestContent content) { - if (manifest_location.empty()) { - return InvalidArgument("Manifest location cannot be empty"); - } - if (!file_io) { - return InvalidArgument("FileIO cannot be null"); - } - if (!partition_spec) { - return InvalidArgument("PartitionSpec cannot be null"); - } - if (!current_schema) { - return InvalidArgument("Current schema cannot be null"); + switch (format_version) { + case kFormatVersion1: { + adapter = std::make_unique( + snapshot_id, std::move(partition_spec), std::move(current_schema)); + break; + } + case kFormatVersion2: { + adapter = std::make_unique( + snapshot_id, std::move(partition_spec), std::move(current_schema), content); + break; + } + case kFormatVersion3: { + adapter = std::make_unique( + snapshot_id, first_row_id, std::move(partition_spec), std::move(current_schema), + content); + writer_first_row_id = first_row_id; + break; + } + default: + return NotSupported("Format version {} is not supported", format_version); } - auto adapter = std::make_unique( - snapshot_id, std::move(partition_spec), std::move(current_schema), content); - ICEBERG_RETURN_UNEXPECTED(adapter->Init()); - ICEBERG_RETURN_UNEXPECTED(adapter->StartAppending()); - - auto schema = adapter->schema(); - ICEBERG_ASSIGN_OR_RAISE( - auto writer, - OpenFileWriter(manifest_location, std::move(schema), std::move(file_io), - adapter->metadata(), "manifest_entry")); - return std::unique_ptr(new ManifestWriter( - std::move(writer), std::move(adapter), manifest_location, std::nullopt)); -} -Result> ManifestWriter::MakeV3Writer( - std::optional snapshot_id, std::optional first_row_id, - std::string_view manifest_location, std::shared_ptr file_io, - std::shared_ptr partition_spec, std::shared_ptr current_schema, - ManifestContent content) { - if (manifest_location.empty()) { - return InvalidArgument("Manifest location cannot be empty"); - } - if (!file_io) { - return InvalidArgument("FileIO cannot be null"); - } - if (!partition_spec) { - return InvalidArgument("PartitionSpec cannot be null"); - } - if (!current_schema) { - return InvalidArgument("Current schema cannot be null"); - } - auto adapter = std::make_unique( - snapshot_id, first_row_id, std::move(partition_spec), std::move(current_schema), - content); ICEBERG_RETURN_UNEXPECTED(adapter->Init()); ICEBERG_RETURN_UNEXPECTED(adapter->StartAppending()); @@ -362,28 +315,7 @@ Result> ManifestWriter::MakeV3Writer( OpenFileWriter(manifest_location, std::move(schema), std::move(file_io), adapter->metadata(), "manifest_entry")); return std::unique_ptr(new ManifestWriter( - std::move(writer), std::move(adapter), manifest_location, first_row_id)); -} - -Result> ManifestWriter::MakeWriter( - int8_t format_version, std::optional snapshot_id, - std::string_view manifest_location, std::shared_ptr file_io, - std::shared_ptr partition_spec, std::shared_ptr current_schema, - ManifestContent content, std::optional first_row_id) { - switch (format_version) { - case 1: - return MakeV1Writer(snapshot_id, manifest_location, std::move(file_io), - std::move(partition_spec), std::move(current_schema)); - case 2: - return MakeV2Writer(snapshot_id, manifest_location, std::move(file_io), - std::move(partition_spec), std::move(current_schema), content); - case 3: - return MakeV3Writer(snapshot_id, first_row_id, manifest_location, - std::move(file_io), std::move(partition_spec), - std::move(current_schema), content); - default: - return NotSupported("Format version {} is not supported", format_version); - } + std::move(writer), std::move(adapter), manifest_location, writer_first_row_id)); } ManifestListWriter::ManifestListWriter(std::unique_ptr writer, @@ -420,83 +352,47 @@ std::optional ManifestListWriter::next_row_id() const { return adapter_->next_row_id(); } -Result> ManifestListWriter::MakeV1Writer( - int64_t snapshot_id, std::optional parent_snapshot_id, - std::string_view manifest_list_location, std::shared_ptr file_io) { - auto adapter = std::make_unique(snapshot_id, parent_snapshot_id); - ICEBERG_RETURN_UNEXPECTED(adapter->Init()); - ICEBERG_RETURN_UNEXPECTED(adapter->StartAppending()); - - auto schema = adapter->schema(); - ICEBERG_ASSIGN_OR_RAISE( - auto writer, - OpenFileWriter(manifest_list_location, std::move(schema), std::move(file_io), - adapter->metadata(), "manifest_file")); - return std::unique_ptr( - new ManifestListWriter(std::move(writer), std::move(adapter))); -} - -Result> ManifestListWriter::MakeV2Writer( - int64_t snapshot_id, std::optional parent_snapshot_id, - int64_t sequence_number, std::string_view manifest_list_location, - std::shared_ptr file_io) { - auto adapter = std::make_unique(snapshot_id, parent_snapshot_id, - sequence_number); - ICEBERG_RETURN_UNEXPECTED(adapter->Init()); - ICEBERG_RETURN_UNEXPECTED(adapter->StartAppending()); - - auto schema = adapter->schema(); - ICEBERG_ASSIGN_OR_RAISE( - auto writer, - OpenFileWriter(manifest_list_location, std::move(schema), std::move(file_io), - adapter->metadata(), "manifest_file")); - - return std::unique_ptr( - new ManifestListWriter(std::move(writer), std::move(adapter))); -} - -Result> ManifestListWriter::MakeV3Writer( - int64_t snapshot_id, std::optional parent_snapshot_id, - int64_t sequence_number, int64_t first_row_id, - std::string_view manifest_list_location, std::shared_ptr file_io) { - auto adapter = std::make_unique(snapshot_id, parent_snapshot_id, - sequence_number, first_row_id); - ICEBERG_RETURN_UNEXPECTED(adapter->Init()); - ICEBERG_RETURN_UNEXPECTED(adapter->StartAppending()); - - auto schema = adapter->schema(); - ICEBERG_ASSIGN_OR_RAISE( - auto writer, - OpenFileWriter(manifest_list_location, std::move(schema), std::move(file_io), - adapter->metadata(), "manifest_file")); - return std::unique_ptr( - new ManifestListWriter(std::move(writer), std::move(adapter))); -} - Result> ManifestListWriter::MakeWriter( int8_t format_version, int64_t snapshot_id, std::optional parent_snapshot_id, std::string_view manifest_list_location, std::shared_ptr file_io, std::optional sequence_number, std::optional first_row_id) { + std::unique_ptr adapter; + switch (format_version) { - case 1: - return MakeV1Writer(snapshot_id, parent_snapshot_id, manifest_list_location, - std::move(file_io)); - case 2: + case kFormatVersion1: { + adapter = std::make_unique(snapshot_id, parent_snapshot_id); + break; + } + case kFormatVersion2: { ICEBERG_PRECHECK(sequence_number.has_value(), "Sequence number is required for format version 2"); - return MakeV2Writer(snapshot_id, parent_snapshot_id, sequence_number.value(), - manifest_list_location, std::move(file_io)); - case 3: + adapter = std::make_unique(snapshot_id, parent_snapshot_id, + sequence_number.value()); + break; + } + case kFormatVersion3: { ICEBERG_PRECHECK(sequence_number.has_value(), "Sequence number is required for format version 3"); ICEBERG_PRECHECK(first_row_id.has_value(), "First row ID is required for format version 3"); - return MakeV3Writer(snapshot_id, parent_snapshot_id, sequence_number.value(), - first_row_id.value(), manifest_list_location, - std::move(file_io)); + adapter = std::make_unique( + snapshot_id, parent_snapshot_id, sequence_number.value(), first_row_id.value()); + break; + } default: return NotSupported("Format version {} is not supported", format_version); } + + ICEBERG_RETURN_UNEXPECTED(adapter->Init()); + ICEBERG_RETURN_UNEXPECTED(adapter->StartAppending()); + + auto schema = adapter->schema(); + ICEBERG_ASSIGN_OR_RAISE( + auto writer, + OpenFileWriter(manifest_list_location, std::move(schema), std::move(file_io), + adapter->metadata(), "manifest_file")); + return std::unique_ptr( + new ManifestListWriter(std::move(writer), std::move(adapter))); } } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_writer.h b/src/iceberg/manifest/manifest_writer.h index 288bda31a..cc57f25fc 100644 --- a/src/iceberg/manifest/manifest_writer.h +++ b/src/iceberg/manifest/manifest_writer.h @@ -117,48 +117,6 @@ class ICEBERG_EXPORT ManifestWriter { /// \note Only valid after the file is closed. Result ToManifestFile() const; - /// \brief Creates a writer for a manifest file. - /// \param snapshot_id ID of the snapshot. - /// \param manifest_location Path to the manifest file. - /// \param file_io File IO implementation to use. - /// \param partition_spec Partition spec for the manifest. - /// \param current_schema Current table schema. - /// \return A Result containing the writer or an error. - static Result> MakeV1Writer( - std::optional snapshot_id, std::string_view manifest_location, - std::shared_ptr file_io, std::shared_ptr partition_spec, - std::shared_ptr current_schema); - - /// \brief Creates a writer for a manifest file. - /// \param snapshot_id ID of the snapshot. - /// \param manifest_location Path to the manifest file. - /// \param file_io File IO implementation to use. - /// \param partition_spec Partition spec for the manifest. - /// \param current_schema Schema containing the source fields referenced by partition - /// spec. - /// \param content Content of the manifest. - /// \return A Result containing the writer or an error. - static Result> MakeV2Writer( - std::optional snapshot_id, std::string_view manifest_location, - std::shared_ptr file_io, std::shared_ptr partition_spec, - std::shared_ptr current_schema, ManifestContent content); - - /// \brief Creates a writer for a manifest file. - /// \param snapshot_id ID of the snapshot. - /// \param first_row_id First row ID of the snapshot. - /// \param manifest_location Path to the manifest file. - /// \param file_io File IO implementation to use. - /// \param partition_spec Partition spec for the manifest. - /// \param current_schema Schema containing the source fields referenced by partition - /// spec. - /// \param content Content of the manifest. - /// \return A Result containing the writer or an error. - static Result> MakeV3Writer( - std::optional snapshot_id, std::optional first_row_id, - std::string_view manifest_location, std::shared_ptr file_io, - std::shared_ptr partition_spec, - std::shared_ptr current_schema, ManifestContent content); - /// \brief Factory function to create a writer for a manifest file based on format /// version. /// \param format_version The format version (1, 2, 3, etc.). @@ -226,41 +184,6 @@ class ICEBERG_EXPORT ManifestListWriter { /// \brief Get the next row id to assign. std::optional next_row_id() const; - /// \brief Creates a writer for the v1 manifest list. - /// \param snapshot_id ID of the snapshot. - /// \param parent_snapshot_id ID of the parent snapshot. - /// \param manifest_list_location Path to the manifest list file. - /// \param file_io File IO implementation to use. - /// \return A Result containing the writer or an error. - static Result> MakeV1Writer( - int64_t snapshot_id, std::optional parent_snapshot_id, - std::string_view manifest_list_location, std::shared_ptr file_io); - - /// \brief Creates a writer for the manifest list. - /// \param snapshot_id ID of the snapshot. - /// \param parent_snapshot_id ID of the parent snapshot. - /// \param sequence_number Sequence number of the snapshot. - /// \param manifest_list_location Path to the manifest list file. - /// \param file_io File IO implementation to use. - /// \return A Result containing the writer or an error. - static Result> MakeV2Writer( - int64_t snapshot_id, std::optional parent_snapshot_id, - int64_t sequence_number, std::string_view manifest_list_location, - std::shared_ptr file_io); - - /// \brief Creates a writer for the manifest list. - /// \param snapshot_id ID of the snapshot. - /// \param parent_snapshot_id ID of the parent snapshot. - /// \param sequence_number Sequence number of the snapshot. - /// \param first_row_id First row ID of the snapshot. - /// \param manifest_list_location Path to the manifest list file. - /// \param file_io File IO implementation to use. - /// \return A Result containing the writer or an error. - static Result> MakeV3Writer( - int64_t snapshot_id, std::optional parent_snapshot_id, - int64_t sequence_number, int64_t first_row_id, - std::string_view manifest_list_location, std::shared_ptr file_io); - /// \brief Factory function to create a writer for the manifest list based on format /// version. /// \param format_version The format version (1, 2, 3, etc.). diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index d3b5629c8..97005ebdd 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -293,7 +293,8 @@ Result> TableMetadata::SnapshotById(int64_t snapshot_i } int64_t TableMetadata::NextSequenceNumber() const { - return format_version > 1 ? last_sequence_number + 1 : kInitialSequenceNumber; + return format_version > kFormatVersion1 ? last_sequence_number + 1 + : kInitialSequenceNumber; } namespace { @@ -856,9 +857,10 @@ Result TableMetadataBuilder::Impl::AddPartitionSpec(const PartitionSpec // Get current schema and validate the partition spec against it ICEBERG_ASSIGN_OR_RAISE(auto schema, metadata_.Schema()); ICEBERG_RETURN_UNEXPECTED(spec.Validate(*schema, /*allow_missing_fields=*/false)); - ICEBERG_CHECK( - metadata_.format_version > 1 || PartitionSpec::HasSequentialFieldIds(spec), - "Spec does not use sequential IDs that are required in v1: {}", spec.ToString()); + ICEBERG_CHECK(metadata_.format_version > kFormatVersion1 || + PartitionSpec::HasSequentialFieldIds(spec), + "Spec does not use sequential IDs that are required in v1: {}", + spec.ToString()); ICEBERG_ASSIGN_OR_RAISE( std::shared_ptr new_spec, @@ -1060,7 +1062,7 @@ Status TableMetadataBuilder::Impl::AddSnapshot(std::shared_ptr snapsho ICEBERG_CHECK(!snapshots_by_id_.contains(snapshot->snapshot_id), "Snapshot already exists for id: {}", snapshot->snapshot_id); ICEBERG_CHECK( - metadata_.format_version == 1 || + metadata_.format_version == kFormatVersion1 || snapshot->sequence_number > metadata_.last_sequence_number || !snapshot->parent_snapshot_id.has_value(), "Cannot add snapshot with sequence number {} older than last sequence number {}", @@ -1127,7 +1129,7 @@ Status TableMetadataBuilder::Impl::SetBranchSnapshotInternal(const Snapshot& sna } ICEBERG_CHECK( - metadata_.format_version == 1 || + metadata_.format_version == kFormatVersion1 || snapshot.sequence_number <= metadata_.last_sequence_number, "Last sequence number {} is less than existing snapshot sequence number {}", metadata_.last_sequence_number, snapshot.sequence_number); diff --git a/src/iceberg/test/delete_file_index_test.cc b/src/iceberg/test/delete_file_index_test.cc index d5866e27c..2c74d5023 100644 --- a/src/iceberg/test/delete_file_index_test.cc +++ b/src/iceberg/test/delete_file_index_test.cc @@ -44,7 +44,7 @@ namespace iceberg { -class DeleteFileIndexTest : public testing::TestWithParam { +class DeleteFileIndexTest : public testing::TestWithParam { protected: void SetUp() override { avro::RegisterAll(); @@ -160,7 +160,7 @@ class DeleteFileIndexTest : public testing::TestWithParam { }; } - ManifestFile WriteDeleteManifest(int format_version, int64_t snapshot_id, + ManifestFile WriteDeleteManifest(int8_t format_version, int64_t snapshot_id, std::vector entries, std::shared_ptr spec) { const std::string manifest_path = MakeManifestPath(); @@ -230,7 +230,7 @@ TEST_P(DeleteFileIndexTest, TestEmptyIndex) { } TEST_P(DeleteFileIndexTest, TestMinSequenceNumberFilteringForFiles) { - int version = GetParam(); + auto version = GetParam(); auto eq_delete_1 = MakeEqualityDeleteFile("/path/to/eq-delete-1.parquet", PartitionValues(std::vector{}), @@ -261,7 +261,7 @@ TEST_P(DeleteFileIndexTest, TestMinSequenceNumberFilteringForFiles) { } TEST_P(DeleteFileIndexTest, TestUnpartitionedDeletes) { - int version = GetParam(); + auto version = GetParam(); auto eq_delete_1 = MakeEqualityDeleteFile("/path/to/eq-delete-1.parquet", PartitionValues(std::vector{}), @@ -359,7 +359,7 @@ TEST_P(DeleteFileIndexTest, TestUnpartitionedDeletes) { } TEST_P(DeleteFileIndexTest, TestPartitionedDeleteIndex) { - int version = GetParam(); + auto version = GetParam(); auto partition_a = PartitionValues({Literal::Int(0)}); auto eq_delete_1 = MakeEqualityDeleteFile("/path/to/eq-delete-1.parquet", partition_a, @@ -462,7 +462,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedDeleteIndex) { } TEST_P(DeleteFileIndexTest, TestPartitionedTableWithPartitionPosDeletes) { - int version = GetParam(); + auto version = GetParam(); auto partition_a = PartitionValues({Literal::Int(0)}); auto pos_delete = MakePositionDeleteFile("/path/to/pos-delete.parquet", partition_a, @@ -487,7 +487,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableWithPartitionPosDeletes) { } TEST_P(DeleteFileIndexTest, TestPartitionedTableWithPartitionEqDeletes) { - int version = GetParam(); + auto version = GetParam(); auto partition_a = PartitionValues({Literal::Int(0)}); auto eq_delete = MakeEqualityDeleteFile("/path/to/eq-delete.parquet", partition_a, @@ -512,7 +512,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableWithPartitionEqDeletes) { } TEST_P(DeleteFileIndexTest, TestPartitionedTableWithUnrelatedPartitionDeletes) { - int version = GetParam(); + auto version = GetParam(); // Create deletes for partition A auto partition_a = PartitionValues({Literal::Int(0)}); @@ -538,8 +538,8 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableWithUnrelatedPartitionDeletes) { } TEST_P(DeleteFileIndexTest, TestPartitionedTableWithOlderPartitionDeletes) { - int version = GetParam(); - if (version >= 3) { + auto version = GetParam(); + if (version >= kFormatVersion3) { GTEST_SKIP() << "DVs are not filtered using sequence numbers in V3+"; } @@ -568,8 +568,8 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableWithOlderPartitionDeletes) { } TEST_P(DeleteFileIndexTest, TestPartitionedTableScanWithGlobalDeletes) { - int version = GetParam(); - if (version >= 3) { + auto version = GetParam(); + if (version >= kFormatVersion3) { GTEST_SKIP() << "Different behavior for position deletes in V3"; } @@ -599,8 +599,8 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableScanWithGlobalDeletes) { } TEST_P(DeleteFileIndexTest, TestPartitionedTableScanWithGlobalAndPartitionDeletes) { - int version = GetParam(); - if (version >= 3) { + auto version = GetParam(); + if (version >= kFormatVersion3) { GTEST_SKIP() << "Different behavior for position deletes in V3"; } @@ -644,7 +644,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableScanWithGlobalAndPartitionDelete } TEST_P(DeleteFileIndexTest, TestPartitionedTableSequenceNumbers) { - int version = GetParam(); + auto version = GetParam(); auto partition_a = PartitionValues({Literal::Int(0)}); auto eq_delete = MakeEqualityDeleteFile("/path/to/eq-delete.parquet", partition_a, @@ -672,8 +672,8 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableSequenceNumbers) { } TEST_P(DeleteFileIndexTest, TestUnpartitionedTableSequenceNumbers) { - int version = GetParam(); - if (version >= 3) { + auto version = GetParam(); + if (version >= kFormatVersion3) { GTEST_SKIP() << "Different behavior in V3"; } @@ -841,8 +841,8 @@ TEST_P(DeleteFileIndexTest, TestEqualityDeletesGroup) { } TEST_P(DeleteFileIndexTest, TestMixDeleteFilesAndDVs) { - int version = GetParam(); - if (version < 3) { + auto version = GetParam(); + if (version < kFormatVersion3) { GTEST_SKIP() << "DVs only supported in V3+"; } @@ -897,8 +897,8 @@ TEST_P(DeleteFileIndexTest, TestMixDeleteFilesAndDVs) { } TEST_P(DeleteFileIndexTest, TestMultipleDVs) { - int version = GetParam(); - if (version < 3) { + auto version = GetParam(); + if (version < kFormatVersion3) { GTEST_SKIP() << "DVs only supported in V3+"; } @@ -923,8 +923,8 @@ TEST_P(DeleteFileIndexTest, TestMultipleDVs) { } TEST_P(DeleteFileIndexTest, TestInvalidDVSequenceNumber) { - int version = GetParam(); - if (version < 3) { + auto version = GetParam(); + if (version < kFormatVersion3) { GTEST_SKIP() << "DVs only supported in V3+"; } @@ -949,7 +949,7 @@ TEST_P(DeleteFileIndexTest, TestInvalidDVSequenceNumber) { } TEST_P(DeleteFileIndexTest, TestReferencedDeleteFiles) { - int version = GetParam(); + auto version = GetParam(); auto partition_a = PartitionValues({Literal::Int(0)}); auto eq_delete = MakeEqualityDeleteFile("/path/to/eq-delete.parquet", partition_a, @@ -987,7 +987,7 @@ TEST_P(DeleteFileIndexTest, TestReferencedDeleteFiles) { } TEST_P(DeleteFileIndexTest, TestExistingDeleteFiles) { - int version = GetParam(); + auto version = GetParam(); auto partition_a = PartitionValues({Literal::Int(0)}); auto eq_delete = MakeEqualityDeleteFile("/path/to/eq-delete.parquet", partition_a, @@ -1020,7 +1020,7 @@ TEST_P(DeleteFileIndexTest, TestExistingDeleteFiles) { } TEST_P(DeleteFileIndexTest, TestDeletedStatusExcluded) { - int version = GetParam(); + auto version = GetParam(); auto partition_a = PartitionValues({Literal::Int(0)}); auto eq_delete_added = MakeEqualityDeleteFile( @@ -1059,7 +1059,7 @@ TEST_P(DeleteFileIndexTest, TestDeletedStatusExcluded) { } TEST_P(DeleteFileIndexTest, TestPositionDeleteDiscardMetrics) { - int version = GetParam(); + auto version = GetParam(); auto partition_a = PartitionValues({Literal::Int(0)}); @@ -1119,7 +1119,7 @@ TEST_P(DeleteFileIndexTest, TestPositionDeleteDiscardMetrics) { } TEST_P(DeleteFileIndexTest, TestEqualityDeleteDiscardMetrics) { - int version = GetParam(); + auto version = GetParam(); auto partition_a = PartitionValues({Literal::Int(0)}); @@ -1177,6 +1177,6 @@ TEST_P(DeleteFileIndexTest, TestEqualityDeleteDiscardMetrics) { } INSTANTIATE_TEST_SUITE_P(DeleteFileIndexVersions, DeleteFileIndexTest, - testing::Values(2, 3)); + testing::Values(kFormatVersion2, kFormatVersion3)); } // namespace iceberg diff --git a/src/iceberg/test/manifest_group_test.cc b/src/iceberg/test/manifest_group_test.cc index 2a062c9ee..60698d449 100644 --- a/src/iceberg/test/manifest_group_test.cc +++ b/src/iceberg/test/manifest_group_test.cc @@ -45,7 +45,7 @@ namespace iceberg { -class ManifestGroupTest : public testing::TestWithParam { +class ManifestGroupTest : public testing::TestWithParam { protected: void SetUp() override { avro::RegisterAll(); @@ -130,7 +130,7 @@ class ManifestGroupTest : public testing::TestWithParam { }; } - ManifestFile WriteDataManifest(int format_version, int64_t snapshot_id, + ManifestFile WriteDataManifest(int8_t format_version, int64_t snapshot_id, std::vector entries, std::shared_ptr spec) { const std::string manifest_path = MakeManifestPath(); @@ -152,7 +152,7 @@ class ManifestGroupTest : public testing::TestWithParam { return std::move(manifest_result.value()); } - ManifestFile WriteDeleteManifest(int format_version, int64_t snapshot_id, + ManifestFile WriteDeleteManifest(int8_t format_version, int64_t snapshot_id, std::vector entries, std::shared_ptr spec) { const std::string manifest_path = MakeManifestPath(); @@ -187,7 +187,7 @@ class ManifestGroupTest : public testing::TestWithParam { // Write a ManifestFile to a manifest list and read it back. This is useful for v1 // to populate all missing fields like sequence_number. - ManifestFile WriteAndReadManifestListEntry(int format_version, int64_t snapshot_id, + ManifestFile WriteAndReadManifestListEntry(int8_t format_version, int64_t snapshot_id, int64_t sequence_number, const ManifestFile& manifest) { const std::string manifest_list_path = MakeManifestListPath(); @@ -236,8 +236,8 @@ class ManifestGroupTest : public testing::TestWithParam { }; TEST_P(ManifestGroupTest, CreateAndGetEntries) { - int version = GetParam(); - if (version < 2) { + auto version = GetParam(); + if (version < kFormatVersion2) { GTEST_SKIP() << "Delete files only supported in V2+"; } @@ -291,7 +291,7 @@ TEST_P(ManifestGroupTest, CreateAndGetEntries) { } TEST_P(ManifestGroupTest, IgnoreDeleted) { - int version = GetParam(); + auto version = GetParam(); constexpr int64_t kSnapshotId = 1000L; constexpr int64_t kSequenceNumber = 1L; @@ -329,7 +329,7 @@ TEST_P(ManifestGroupTest, IgnoreDeleted) { } TEST_P(ManifestGroupTest, IgnoreExisting) { - int version = GetParam(); + auto version = GetParam(); constexpr int64_t kSnapshotId = 1000L; constexpr int64_t kSequenceNumber = 1L; @@ -367,7 +367,7 @@ TEST_P(ManifestGroupTest, IgnoreExisting) { } TEST_P(ManifestGroupTest, CustomManifestEntriesFilter) { - int version = GetParam(); + auto version = GetParam(); constexpr int64_t kSnapshotId = 1000L; const auto part_value = PartitionValues({Literal::Int(0)}); @@ -418,7 +418,7 @@ TEST_P(ManifestGroupTest, EmptyManifestGroup) { } TEST_P(ManifestGroupTest, MultipleDataManifests) { - int version = GetParam(); + auto version = GetParam(); const auto partition_a = PartitionValues({Literal::Int(0)}); const auto partition_b = PartitionValues({Literal::Int(1)}); @@ -451,7 +451,7 @@ TEST_P(ManifestGroupTest, MultipleDataManifests) { } TEST_P(ManifestGroupTest, PartitionFilter) { - int version = GetParam(); + auto version = GetParam(); // Create two files with different partition values (bucket 0 and bucket 1) const auto partition_bucket_0 = PartitionValues({Literal::Int(0)}); @@ -485,6 +485,7 @@ TEST_P(ManifestGroupTest, PartitionFilter) { } INSTANTIATE_TEST_SUITE_P(ManifestGroupVersions, ManifestGroupTest, - testing::Values(1, 2, 3)); + testing::Values(kFormatVersion1, kFormatVersion2, + kFormatVersion3)); } // namespace iceberg diff --git a/src/iceberg/test/manifest_list_versions_test.cc b/src/iceberg/test/manifest_list_versions_test.cc index f7049fad6..3b20fc69b 100644 --- a/src/iceberg/test/manifest_list_versions_test.cc +++ b/src/iceberg/test/manifest_list_versions_test.cc @@ -103,7 +103,7 @@ class TestManifestListVersions : public ::testing::Test { std::chrono::system_clock::now().time_since_epoch().count()); } - std::string WriteManifestList(int format_version, int64_t expected_next_row_id, + std::string WriteManifestList(int8_t format_version, int64_t expected_next_row_id, const std::vector& manifests) const { const std::string manifest_list_path = CreateManifestListPath(); constexpr int64_t kParentSnapshotId = kSnapshotId - 1; @@ -126,7 +126,7 @@ class TestManifestListVersions : public ::testing::Test { return manifest_list_path; } - ManifestFile WriteAndReadManifestList(int format_version) const { + ManifestFile WriteAndReadManifestList(int8_t format_version) const { return ReadManifestList( WriteManifestList(format_version, kSnapshotFirstRowId, {kTestManifest})); } @@ -190,9 +190,9 @@ class TestManifestListVersions : public ::testing::Test { TEST_F(TestManifestListVersions, TestV1WriteDeleteManifest) { const std::string manifest_list_path = CreateManifestListPath(); - ICEBERG_UNWRAP_OR_FAIL(auto writer, - ManifestListWriter::MakeV1Writer(kSnapshotId, kSnapshotId - 1, - manifest_list_path, file_io_)); + ICEBERG_UNWRAP_OR_FAIL(auto writer, ManifestListWriter::MakeWriter( + kFormatVersion1, kSnapshotId, kSnapshotId - 1, + manifest_list_path, file_io_)); auto status = writer->Add(kDeleteManifest); EXPECT_THAT(status, IsError(ErrorKind::kInvalidManifestList)); @@ -200,7 +200,7 @@ TEST_F(TestManifestListVersions, TestV1WriteDeleteManifest) { } TEST_F(TestManifestListVersions, TestV1Write) { - auto manifest = WriteAndReadManifestList(/*format_version=*/1); + auto manifest = WriteAndReadManifestList(kFormatVersion1); // V3 fields are not written and are defaulted EXPECT_FALSE(manifest.first_row_id.has_value()); @@ -224,7 +224,7 @@ TEST_F(TestManifestListVersions, TestV1Write) { } TEST_F(TestManifestListVersions, TestV2Write) { - auto manifest = WriteAndReadManifestList(2); + auto manifest = WriteAndReadManifestList(kFormatVersion2); // V3 fields are not written and are defaulted EXPECT_FALSE(manifest.first_row_id.has_value()); @@ -246,7 +246,7 @@ TEST_F(TestManifestListVersions, TestV2Write) { } TEST_F(TestManifestListVersions, TestV3Write) { - auto manifest = WriteAndReadManifestList(/*format_version=*/3); + auto manifest = WriteAndReadManifestList(kFormatVersion3); // All V3 fields should be read correctly EXPECT_EQ(manifest.manifest_path, kPath); @@ -272,7 +272,7 @@ TEST_F(TestManifestListVersions, TestV3WriteFirstRowIdAssignment) { constexpr int64_t kExpectedNextRowId = kSnapshotFirstRowId + kAddedRows + kExistingRows; auto manifest_list_path = - WriteManifestList(/*format_version=*/3, kExpectedNextRowId, {missing_first_row_id}); + WriteManifestList(kFormatVersion3, kExpectedNextRowId, {missing_first_row_id}); auto manifest = ReadManifestList(manifest_list_path); EXPECT_EQ(manifest.manifest_path, kPath); @@ -298,8 +298,9 @@ TEST_F(TestManifestListVersions, TestV3WriteMixedRowIdAssignment) { constexpr int64_t kExpectedNextRowId = kSnapshotFirstRowId + 2 * (kAddedRows + kExistingRows); - auto manifest_list_path = WriteManifestList( - 3, kExpectedNextRowId, {missing_first_row_id, kTestManifest, missing_first_row_id}); + auto manifest_list_path = + WriteManifestList(kFormatVersion3, kExpectedNextRowId, + {missing_first_row_id, kTestManifest, missing_first_row_id}); auto manifests = ReadAllManifests(manifest_list_path); EXPECT_EQ(manifests.size(), 3); @@ -329,7 +330,7 @@ TEST_F(TestManifestListVersions, TestV3WriteMixedRowIdAssignment) { TEST_F(TestManifestListVersions, TestV1ForwardCompatibility) { std::string manifest_list_path = - WriteManifestList(/*format_version=*/1, kSnapshotFirstRowId, {kTestManifest}); + WriteManifestList(kFormatVersion1, kSnapshotFirstRowId, {kTestManifest}); std::string expected_array_json = R"([ ["s3://bucket/table/m1.avro", 1024, 1, 987134631982734, 2, 343, 1, [], 5292, 857273, 22910, null] ])"; @@ -341,7 +342,7 @@ TEST_F(TestManifestListVersions, TestV2ForwardCompatibility) { // V2 manifest list files can be read by V1 readers, but the sequence numbers and // content will be ignored. std::string manifest_list_path = - WriteManifestList(/*format_version=*/2, kSnapshotFirstRowId, {kTestManifest}); + WriteManifestList(kFormatVersion2, kSnapshotFirstRowId, {kTestManifest}); std::string expected_array_json = R"([ ["s3://bucket/table/m1.avro", 1024, 1, 987134631982734, 2, 343, 1, [], 5292, 857273, 22910, null] ])"; @@ -444,7 +445,8 @@ TEST_F(TestManifestListVersions, TestManifestsPartitionSummary) { }; // Test for all format versions - for (int format_version = 1; format_version <= 3; ++format_version) { + for (int8_t format_version = kFormatVersion1; format_version <= kFormatVersion3; + ++format_version) { int64_t expected_next_row_id = kSnapshotFirstRowId + manifest.added_rows_count.value() + manifest.existing_rows_count.value(); diff --git a/src/iceberg/test/manifest_reader_stats_test.cc b/src/iceberg/test/manifest_reader_stats_test.cc index d1da17953..ff7767e7a 100644 --- a/src/iceberg/test/manifest_reader_stats_test.cc +++ b/src/iceberg/test/manifest_reader_stats_test.cc @@ -40,7 +40,7 @@ namespace iceberg { -class TestManifestReaderStats : public testing::TestWithParam { +class TestManifestReaderStats : public testing::TestWithParam { protected: void SetUp() override { avro::RegisterAll(); @@ -86,7 +86,7 @@ class TestManifestReaderStats : public testing::TestWithParam { }); } - ManifestFile WriteManifest(int format_version, std::unique_ptr data_file) { + ManifestFile WriteManifest(int8_t format_version, std::unique_ptr data_file) { const std::string manifest_path = MakeManifestPath(); auto writer_result = ManifestWriter::MakeWriter( @@ -127,7 +127,7 @@ class TestManifestReaderStats : public testing::TestWithParam { }; TEST_P(TestManifestReaderStats, TestReadIncludesFullStats) { - int version = GetParam(); + auto version = GetParam(); auto manifest = WriteManifest(version, MakeDataFileWithStats()); auto reader_result = ManifestReader::Make(manifest, file_io_, schema_, spec_); @@ -142,7 +142,7 @@ TEST_P(TestManifestReaderStats, TestReadIncludesFullStats) { } TEST_P(TestManifestReaderStats, TestReadEntriesWithFilterIncludesFullStats) { - int version = GetParam(); + auto version = GetParam(); auto manifest = WriteManifest(version, MakeDataFileWithStats()); auto reader_result = ManifestReader::Make(manifest, file_io_, schema_, spec_); @@ -159,7 +159,7 @@ TEST_P(TestManifestReaderStats, TestReadEntriesWithFilterIncludesFullStats) { } TEST_P(TestManifestReaderStats, TestReadEntriesWithFilterAndSelectIncludesFullStats) { - int version = GetParam(); + auto version = GetParam(); auto manifest = WriteManifest(version, MakeDataFileWithStats()); auto reader_result = ManifestReader::Make(manifest, file_io_, schema_, spec_); @@ -177,7 +177,7 @@ TEST_P(TestManifestReaderStats, TestReadEntriesWithFilterAndSelectIncludesFullSt } TEST_P(TestManifestReaderStats, TestReadEntriesWithSelectNotProjectStats) { - int version = GetParam(); + auto version = GetParam(); auto manifest = WriteManifest(version, MakeDataFileWithStats()); auto reader_result = ManifestReader::Make(manifest, file_io_, schema_, spec_); @@ -194,7 +194,7 @@ TEST_P(TestManifestReaderStats, TestReadEntriesWithSelectNotProjectStats) { } TEST_P(TestManifestReaderStats, TestReadEntriesWithSelectCertainStatNotProjectStats) { - int version = GetParam(); + auto version = GetParam(); auto manifest = WriteManifest(version, MakeDataFileWithStats()); auto reader_result = ManifestReader::Make(manifest, file_io_, schema_, spec_); @@ -217,6 +217,7 @@ TEST_P(TestManifestReaderStats, TestReadEntriesWithSelectCertainStatNotProjectSt } INSTANTIATE_TEST_SUITE_P(ManifestReaderStatsVersions, TestManifestReaderStats, - testing::Values(1, 2, 3)); + testing::Values(kFormatVersion1, kFormatVersion2, + kFormatVersion3)); } // namespace iceberg diff --git a/src/iceberg/test/manifest_reader_test.cc b/src/iceberg/test/manifest_reader_test.cc index 7604eba9f..830e6951f 100644 --- a/src/iceberg/test/manifest_reader_test.cc +++ b/src/iceberg/test/manifest_reader_test.cc @@ -42,7 +42,7 @@ namespace iceberg { -class TestManifestReader : public testing::TestWithParam { +class TestManifestReader : public testing::TestWithParam { protected: void SetUp() override { avro::RegisterAll(); @@ -78,7 +78,7 @@ class TestManifestReader : public testing::TestWithParam { }); } - ManifestFile WriteManifest(int format_version, std::optional snapshot_id, + ManifestFile WriteManifest(int8_t format_version, std::optional snapshot_id, const std::vector& entries) { const std::string manifest_path = MakeManifestPath(); @@ -134,7 +134,7 @@ class TestManifestReader : public testing::TestWithParam { }); } - ManifestFile WriteDeleteManifest(int format_version, int64_t snapshot_id, + ManifestFile WriteDeleteManifest(int8_t format_version, int64_t snapshot_id, std::vector entries) { const std::string manifest_path = MakeManifestPath(); @@ -162,7 +162,7 @@ class TestManifestReader : public testing::TestWithParam { }; TEST_P(TestManifestReader, TestManifestReaderWithEmptyInheritableMetadata) { - int version = GetParam(); + auto version = GetParam(); auto file_a = MakeDataFile("/path/to/data-a.parquet", PartitionValues({Literal::Int(0)})); @@ -191,7 +191,7 @@ TEST_P(TestManifestReader, TestManifestReaderWithEmptyInheritableMetadata) { } TEST_P(TestManifestReader, TestReaderWithFilterWithoutSelect) { - int version = GetParam(); + auto version = GetParam(); auto file_a = MakeDataFile("/path/to/data-a.parquet", PartitionValues({Literal::Int(0)})); auto file_b = @@ -225,7 +225,7 @@ TEST_P(TestManifestReader, TestReaderWithFilterWithoutSelect) { } TEST_P(TestManifestReader, TestManifestReaderWithPartitionMetadata) { - int version = GetParam(); + auto version = GetParam(); auto file_a = MakeDataFile("/path/to/data-a.parquet", PartitionValues({Literal::Int(0)})); auto entry = @@ -254,8 +254,8 @@ TEST_P(TestManifestReader, TestManifestReaderWithPartitionMetadata) { } TEST_P(TestManifestReader, TestDeleteFilesWithReferences) { - int version = GetParam(); - if (version < 2) { + auto version = GetParam(); + if (version < kFormatVersion2) { GTEST_SKIP() << "Delete files only supported in V2+"; } @@ -293,8 +293,8 @@ TEST_P(TestManifestReader, TestDeleteFilesWithReferences) { } TEST_P(TestManifestReader, TestDVs) { - int version = GetParam(); - if (version < 3) { + auto version = GetParam(); + if (version < kFormatVersion3) { GTEST_SKIP() << "DVs only supported in V3+"; } @@ -337,7 +337,7 @@ TEST_P(TestManifestReader, TestDVs) { } TEST_P(TestManifestReader, TestInvalidUsage) { - int version = GetParam(); + auto version = GetParam(); auto file_a = MakeDataFile("/path/to/data-a.parquet", PartitionValues({Literal::Int(0)})); auto entry = @@ -354,7 +354,7 @@ TEST_P(TestManifestReader, TestInvalidUsage) { } TEST_P(TestManifestReader, TestDropStats) { - int version = GetParam(); + auto version = GetParam(); // Create a data file with full metrics auto file_with_stats = std::make_unique(DataFile{ @@ -419,6 +419,7 @@ TEST(ManifestReaderStaticTest, TestShouldDropStats) { } INSTANTIATE_TEST_SUITE_P(ManifestReaderVersions, TestManifestReader, - testing::Values(1, 2, 3)); + testing::Values(kFormatVersion1, kFormatVersion2, + kFormatVersion3)); } // namespace iceberg diff --git a/src/iceberg/test/manifest_writer_versions_test.cc b/src/iceberg/test/manifest_writer_versions_test.cc index cc4e804be..8ba0e6b8c 100644 --- a/src/iceberg/test/manifest_writer_versions_test.cc +++ b/src/iceberg/test/manifest_writer_versions_test.cc @@ -157,7 +157,7 @@ class ManifestWriterVersionsTest : public ::testing::Test { std::chrono::system_clock::now().time_since_epoch().count()); } - std::string WriteManifests(int format_version, + std::string WriteManifests(int8_t format_version, const std::vector& manifests) const { const std::string manifest_list_path = CreateManifestListPath(); constexpr int64_t kParentSnapshotId = kSnapshotId - 1; @@ -175,7 +175,7 @@ class ManifestWriterVersionsTest : public ::testing::Test { } std::vector WriteAndReadManifests( - const std::vector& manifests, int format_version) const { + const std::vector& manifests, int8_t format_version) const { return ReadManifests(WriteManifests(format_version, manifests)); } @@ -195,7 +195,7 @@ class ManifestWriterVersionsTest : public ::testing::Test { std::chrono::system_clock::now().time_since_epoch().count()); } - ManifestFile WriteManifest(int format_version, + ManifestFile WriteManifest(int8_t format_version, std::vector> data_files) { const std::string manifest_path = CreateManifestPath(); @@ -228,7 +228,7 @@ class ManifestWriterVersionsTest : public ::testing::Test { return entries_result.value(); } - ManifestFile WriteDeleteManifest(int format_version, + ManifestFile WriteDeleteManifest(int8_t format_version, std::shared_ptr delete_file) { const std::string manifest_path = CreateManifestPath(); @@ -249,7 +249,7 @@ class ManifestWriterVersionsTest : public ::testing::Test { return std::move(manifest_result.value()); } - ManifestFile RewriteManifest(const ManifestFile& old_manifest, int format_version) { + ManifestFile RewriteManifest(const ManifestFile& old_manifest, int8_t format_version) { auto entries = ReadManifest(old_manifest); const std::string manifest_path = CreateManifestPath(); @@ -411,7 +411,7 @@ class ManifestWriterVersionsTest : public ::testing::Test { }; TEST_F(ManifestWriterVersionsTest, TestV1Write) { - auto manifest = WriteManifest(/*format_version=*/1, {data_file_}); + auto manifest = WriteManifest(kFormatVersion1, {data_file_}); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -422,8 +422,8 @@ TEST_F(ManifestWriterVersionsTest, TestV1Write) { TEST_F(ManifestWriterVersionsTest, TestV1WriteDelete) { const std::string manifest_path = CreateManifestPath(); ICEBERG_UNWRAP_OR_FAIL( - auto writer, - ManifestWriter::MakeV1Writer(kSnapshotId, manifest_path, file_io_, spec_, schema_)); + auto writer, ManifestWriter::MakeWriter(kFormatVersion1, kSnapshotId, manifest_path, + file_io_, spec_, schema_)); ManifestEntry entry; entry.snapshot_id = kSnapshotId; @@ -436,8 +436,8 @@ TEST_F(ManifestWriterVersionsTest, TestV1WriteDelete) { } TEST_F(ManifestWriterVersionsTest, TestV1WriteWithInheritance) { - auto manifests = - WriteAndReadManifests({WriteManifest(/*format_version=*/1, {data_file_})}, 1); + auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion1, {data_file_})}, + kFormatVersion1); ASSERT_EQ(manifests.size(), 1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); @@ -448,7 +448,7 @@ TEST_F(ManifestWriterVersionsTest, TestV1WriteWithInheritance) { } TEST_F(ManifestWriterVersionsTest, TestV2Write) { - auto manifest = WriteManifest(/*format_version=*/2, {data_file_}); + auto manifest = WriteManifest(kFormatVersion2, {data_file_}); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -458,8 +458,8 @@ TEST_F(ManifestWriterVersionsTest, TestV2Write) { } TEST_F(ManifestWriterVersionsTest, TestV2WriteWithInheritance) { - auto manifests = - WriteAndReadManifests({WriteManifest(/*format_version=*/2, {data_file_})}, 2); + auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion2, {data_file_})}, + kFormatVersion2); CheckManifest(manifests[0], kSequenceNumber, kSequenceNumber); auto entries = ReadManifest(manifests[0]); ASSERT_EQ(entries.size(), 1); @@ -468,7 +468,7 @@ TEST_F(ManifestWriterVersionsTest, TestV2WriteWithInheritance) { } TEST_F(ManifestWriterVersionsTest, TestV2PlusWriteDeleteV2) { - auto manifest = WriteDeleteManifest(/*format_version=*/2, delete_file_); + auto manifest = WriteDeleteManifest(kFormatVersion2, delete_file_); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -479,13 +479,13 @@ TEST_F(ManifestWriterVersionsTest, TestV2PlusWriteDeleteV2) { TEST_F(ManifestWriterVersionsTest, TestV2ManifestListRewriteWithInheritance) { // write with v1 - auto manifests = - WriteAndReadManifests({WriteManifest(/*format_version=*/1, {data_file_})}, 1); + auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion1, {data_file_})}, + kFormatVersion1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite existing metadata with v2 manifest list - auto manifests2 = WriteAndReadManifests(manifests, 2); + auto manifests2 = WriteAndReadManifests(manifests, kFormatVersion2); // the ManifestFile did not change and should still have its original sequence number, 0 CheckManifest(manifests2[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); @@ -498,18 +498,18 @@ TEST_F(ManifestWriterVersionsTest, TestV2ManifestListRewriteWithInheritance) { TEST_F(ManifestWriterVersionsTest, TestV2ManifestRewriteWithInheritance) { // write with v1 - auto manifests = - WriteAndReadManifests({WriteManifest(/*format_version=*/1, {data_file_})}, 1); + auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion1, {data_file_})}, + kFormatVersion1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite the manifest file using a v2 manifest - auto rewritten_manifest = RewriteManifest(manifests[0], 2); + auto rewritten_manifest = RewriteManifest(manifests[0], kFormatVersion2); CheckRewrittenManifest(rewritten_manifest, kInvalidSequenceNumber, TableMetadata::kInitialSequenceNumber); // add the v2 manifest to a v2 manifest list, with a sequence number - auto manifests2 = WriteAndReadManifests({rewritten_manifest}, 2); + auto manifests2 = WriteAndReadManifests({rewritten_manifest}, kFormatVersion2); // the ManifestFile is new so it has a sequence number, but the min sequence number 0 is // from the entry CheckRewrittenManifest(manifests2[0], kSequenceNumber, @@ -522,7 +522,7 @@ TEST_F(ManifestWriterVersionsTest, TestV2ManifestRewriteWithInheritance) { } TEST_F(ManifestWriterVersionsTest, TestV3Write) { - auto manifest = WriteManifest(/*format_version=*/3, {data_file_}); + auto manifest = WriteManifest(kFormatVersion3, {data_file_}); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -533,7 +533,8 @@ TEST_F(ManifestWriterVersionsTest, TestV3Write) { TEST_F(ManifestWriterVersionsTest, TestV3WriteWithInheritance) { auto manifests = WriteAndReadManifests( - {WriteManifest(/*format_version=*/3, {data_file_without_first_row_id_})}, 3); + {WriteManifest(kFormatVersion3, {data_file_without_first_row_id_})}, + kFormatVersion3); CheckManifest(manifests[0], kSequenceNumber, kSequenceNumber); ASSERT_EQ(manifests[0].content, ManifestContent::kData); @@ -548,7 +549,7 @@ TEST_F(ManifestWriterVersionsTest, TestV3WriteWithInheritance) { TEST_F(ManifestWriterVersionsTest, TestV3WriteFirstRowIdAssignment) { auto manifests = WriteAndReadManifests( - {WriteManifest(/*format_version=*/3, + {WriteManifest(kFormatVersion3, {data_file_without_first_row_id_, data_file_without_first_row_id_})}, 3); ASSERT_EQ(manifests[0].content, ManifestContent::kData); @@ -567,13 +568,13 @@ TEST_F(ManifestWriterVersionsTest, TestV3WriteFirstRowIdAssignment) { TEST_F(ManifestWriterVersionsTest, TestV3ManifestListRewriteWithInheritance) { // write with v1 - auto manifests = - WriteAndReadManifests({WriteManifest(/*format_version=*/1, {data_file_})}, 1); + auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion1, {data_file_})}, + kFormatVersion1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite existing metadata with a manifest list - auto manifests3 = WriteAndReadManifests(manifests, 3); + auto manifests3 = WriteAndReadManifests(manifests, kFormatVersion3); // the ManifestFile did not change and should still have its original sequence number, 0 CheckManifest(manifests3[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); @@ -587,18 +588,18 @@ TEST_F(ManifestWriterVersionsTest, TestV3ManifestListRewriteWithInheritance) { TEST_F(ManifestWriterVersionsTest, TestV3ManifestRewriteWithInheritance) { // write with v1 - auto manifests = - WriteAndReadManifests({WriteManifest(/*format_version=*/1, {data_file_})}, 1); + auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion1, {data_file_})}, + kFormatVersion1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite the manifest file using a v3 manifest - auto rewritten_manifest = RewriteManifest(manifests[0], 3); + auto rewritten_manifest = RewriteManifest(manifests[0], kFormatVersion3); CheckRewrittenManifest(rewritten_manifest, kInvalidSequenceNumber, TableMetadata::kInitialSequenceNumber); // add the v3 manifest to a v3 manifest list, with a sequence number - auto manifests3 = WriteAndReadManifests({rewritten_manifest}, 3); + auto manifests3 = WriteAndReadManifests({rewritten_manifest}, kFormatVersion3); // the ManifestFile is new so it has a sequence number, but the min sequence number 0 is // from the entry CheckRewrittenManifest(manifests3[0], kSequenceNumber, diff --git a/src/iceberg/test/rolling_manifest_writer_test.cc b/src/iceberg/test/rolling_manifest_writer_test.cc index 439359bc8..0d3a93dcc 100644 --- a/src/iceberg/test/rolling_manifest_writer_test.cc +++ b/src/iceberg/test/rolling_manifest_writer_test.cc @@ -86,7 +86,7 @@ static std::string CreateManifestPath() { } // namespace -class RollingManifestWriterTest : public ::testing::TestWithParam { +class RollingManifestWriterTest : public ::testing::TestWithParam { protected: void SetUp() override { avro::RegisterAll(); @@ -106,7 +106,7 @@ class RollingManifestWriterTest : public ::testing::TestWithParam { } RollingManifestWriter::ManifestWriterFactory NewRollingWriteManifestFactory( - int32_t format_version) { + int8_t format_version) { return [this, format_version]() -> Result> { const std::string manifest_path = CreateManifestPath(); return ManifestWriter::MakeWriter(format_version, kSnapshotId, manifest_path, @@ -116,7 +116,7 @@ class RollingManifestWriterTest : public ::testing::TestWithParam { } RollingManifestWriter::ManifestWriterFactory NewRollingWriteDeleteManifestFactory( - int32_t format_version) { + int8_t format_version) { return [this, format_version]() -> Result> { const std::string manifest_path = CreateManifestPath(); return ManifestWriter::MakeWriter(format_version, kSnapshotId, manifest_path, @@ -155,7 +155,7 @@ class RollingManifestWriterTest : public ::testing::TestWithParam { }; TEST_P(RollingManifestWriterTest, TestRollingManifestWriterNoRecords) { - int32_t format_version = GetParam(); + auto format_version = GetParam(); RollingManifestWriter writer(NewRollingWriteManifestFactory(format_version), kSmallFileSize); @@ -170,8 +170,8 @@ TEST_P(RollingManifestWriterTest, TestRollingManifestWriterNoRecords) { } TEST_P(RollingManifestWriterTest, TestRollingDeleteManifestWriterNoRecords) { - int32_t format_version = GetParam(); - if (format_version < 2) { + auto format_version = GetParam(); + if (format_version < kFormatVersion2) { GTEST_SKIP() << "Delete manifests only supported in V2+"; } RollingManifestWriter writer(NewRollingWriteDeleteManifestFactory(format_version), @@ -188,7 +188,7 @@ TEST_P(RollingManifestWriterTest, TestRollingDeleteManifestWriterNoRecords) { } TEST_P(RollingManifestWriterTest, TestRollingManifestWriterSplitFiles) { - int32_t format_version = GetParam(); + auto format_version = GetParam(); RollingManifestWriter writer(NewRollingWriteManifestFactory(format_version), kSmallFileSize); @@ -239,8 +239,8 @@ TEST_P(RollingManifestWriterTest, TestRollingManifestWriterSplitFiles) { } TEST_P(RollingManifestWriterTest, TestRollingDeleteManifestWriterSplitFiles) { - int32_t format_version = GetParam(); - if (format_version < 2) { + auto format_version = GetParam(); + if (format_version < kFormatVersion2) { GTEST_SKIP() << "Delete manifests only supported in V2+"; } RollingManifestWriter writer(NewRollingWriteDeleteManifestFactory(format_version), @@ -293,6 +293,7 @@ TEST_P(RollingManifestWriterTest, TestRollingDeleteManifestWriterSplitFiles) { } INSTANTIATE_TEST_SUITE_P(TestRollingManifestWriter, RollingManifestWriterTest, - ::testing::Values(1, 2, 3)); + ::testing::Values(kFormatVersion1, kFormatVersion2, + kFormatVersion3)); } // namespace iceberg diff --git a/src/iceberg/test/table_scan_test.cc b/src/iceberg/test/table_scan_test.cc index b496606cf..e669ede64 100644 --- a/src/iceberg/test/table_scan_test.cc +++ b/src/iceberg/test/table_scan_test.cc @@ -47,7 +47,7 @@ namespace iceberg { -class TableScanTest : public testing::TestWithParam { +class TableScanTest : public testing::TestWithParam { protected: void SetUp() override { avro::RegisterAll(); @@ -175,14 +175,15 @@ class TableScanTest : public testing::TestWithParam { }; } - ManifestFile WriteDataManifest(int format_version, int64_t snapshot_id, + ManifestFile WriteDataManifest(int8_t format_version, int64_t snapshot_id, std::vector entries, std::shared_ptr spec) { const std::string manifest_path = MakeManifestPath(); auto writer_result = ManifestWriter::MakeWriter( format_version, snapshot_id, manifest_path, file_io_, spec, schema_, ManifestContent::kData, - /*first_row_id=*/format_version >= 3 ? std::optional(0L) : std::nullopt); + /*first_row_id=*/format_version >= kFormatVersion3 ? std::optional(0L) + : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -197,7 +198,7 @@ class TableScanTest : public testing::TestWithParam { return std::move(manifest_result.value()); } - ManifestFile WriteDeleteManifest(int format_version, int64_t snapshot_id, + ManifestFile WriteDeleteManifest(int8_t format_version, int64_t snapshot_id, std::vector entries, std::shared_ptr spec) { const std::string manifest_path = MakeManifestPath(); @@ -224,7 +225,7 @@ class TableScanTest : public testing::TestWithParam { std::chrono::system_clock::now().time_since_epoch().count()); } - std::string WriteManifestList(int format_version, int64_t snapshot_id, + std::string WriteManifestList(int8_t format_version, int64_t snapshot_id, int64_t sequence_number, const std::vector& manifests) { const std::string manifest_list_path = MakeManifestListPath(); @@ -232,9 +233,11 @@ class TableScanTest : public testing::TestWithParam { auto writer_result = ManifestListWriter::MakeWriter( format_version, snapshot_id, kParentSnapshotId, manifest_list_path, file_io_, - /*sequence_number=*/format_version >= 2 ? std::optional(sequence_number) - : std::nullopt, - /*first_row_id=*/format_version >= 3 ? std::optional(0L) : std::nullopt); + /*sequence_number=*/format_version >= kFormatVersion2 + ? std::optional(sequence_number) + : std::nullopt, + /*first_row_id=*/format_version >= kFormatVersion3 ? std::optional(0L) + : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -367,7 +370,7 @@ TEST_P(TableScanTest, DataTableScanPlanFilesEmpty) { } TEST_P(TableScanTest, PlanFilesWithDataManifests) { - int version = GetParam(); + auto version = GetParam(); constexpr int64_t kSnapshotId = 1000L; const auto part_value = PartitionValues({Literal::Int(0)}); @@ -427,7 +430,7 @@ TEST_P(TableScanTest, PlanFilesWithDataManifests) { } TEST_P(TableScanTest, PlanFilesWithMultipleManifests) { - int version = GetParam(); + auto version = GetParam(); const auto partition_a = PartitionValues({Literal::Int(0)}); const auto partition_b = PartitionValues({Literal::Int(1)}); @@ -494,7 +497,7 @@ TEST_P(TableScanTest, PlanFilesWithMultipleManifests) { } TEST_P(TableScanTest, PlanFilesWithFilter) { - int version = GetParam(); + auto version = GetParam(); constexpr int64_t kSnapshotId = 1000L; const auto part_value = PartitionValues({Literal::Int(0)}); @@ -587,8 +590,8 @@ TEST_P(TableScanTest, PlanFilesWithFilter) { } TEST_P(TableScanTest, PlanFilesWithDeleteFiles) { - int version = GetParam(); - if (version < 2) { + auto version = GetParam(); + if (version < kFormatVersion2) { GTEST_SKIP() << "Delete files only supported in V2+"; } @@ -666,6 +669,8 @@ TEST_P(TableScanTest, PlanFilesWithDeleteFiles) { } } -INSTANTIATE_TEST_SUITE_P(TableScanVersions, TableScanTest, testing::Values(1, 2, 3)); +INSTANTIATE_TEST_SUITE_P(TableScanVersions, TableScanTest, + testing::Values(kFormatVersion1, kFormatVersion2, + kFormatVersion3)); } // namespace iceberg diff --git a/src/iceberg/update/snapshot_update.cc b/src/iceberg/update/snapshot_update.cc index 38c5129f4..1b3ff7839 100644 --- a/src/iceberg/update/snapshot_update.cc +++ b/src/iceberg/update/snapshot_update.cc @@ -157,7 +157,7 @@ SnapshotUpdate::~SnapshotUpdate() = default; SnapshotUpdate::SnapshotUpdate(std::shared_ptr transaction) : PendingUpdate(std::move(transaction)), can_inherit_snapshot_id_( - base().format_version > 1 || + base().format_version > kFormatVersion1 || base().properties.Get(TableProperties::kSnapshotIdInheritanceEnabled)), commit_uuid_(Uuid::GenerateV7().ToString()), target_manifest_size_bytes_( @@ -261,7 +261,7 @@ Result SnapshotUpdate::Apply() { std::optional next_row_id; std::optional assigned_rows; - if (base().format_version >= 3) { + if (base().format_version >= kFormatVersion3) { ICEBERG_CHECK(writer->next_row_id().has_value(), "row id is required by format version >= 3"); next_row_id = base().next_row_id; diff --git a/src/iceberg/update/update_partition_spec.cc b/src/iceberg/update/update_partition_spec.cc index 54c3dc60a..ecb22296c 100644 --- a/src/iceberg/update/update_partition_spec.cc +++ b/src/iceberg/update/update_partition_spec.cc @@ -25,6 +25,7 @@ #include #include +#include "iceberg/constants.h" #include "iceberg/expression/term.h" #include "iceberg/partition_field.h" #include "iceberg/partition_spec.h" @@ -32,7 +33,6 @@ #include "iceberg/table_metadata.h" #include "iceberg/transaction.h" #include "iceberg/transform.h" -#include "iceberg/util/checked_cast.h" #include "iceberg/util/macros.h" namespace iceberg { @@ -81,7 +81,7 @@ UpdatePartitionSpec::UpdatePartitionSpec(std::shared_ptr transactio } // Build index of historical partition fields for efficient recycling (V2+) - if (format_version_ >= 2) { + if (format_version_ >= kFormatVersion2) { BuildHistoricalFieldsIndex(); } } @@ -326,7 +326,7 @@ Result UpdatePartitionSpec::Apply() { } else { new_fields.push_back(field); } - } else if (format_version_ < 2) { + } else if (format_version_ < kFormatVersion2) { // field IDs were not required for v1 and were assigned sequentially in each // partition spec starting at 1,000. // To maintain consistent field ids across partition specs in v1 tables, any @@ -358,7 +358,7 @@ int32_t UpdatePartitionSpec::AssignFieldId() { return ++last_assigned_partition_ PartitionField UpdatePartitionSpec::RecycleOrCreatePartitionField( int32_t source_id, std::shared_ptr transform, std::string_view name) { // In V2+, use pre-built index for O(1) lookup instead of O(n*m) iteration - if (format_version_ >= 2 && !historical_fields_.empty()) { + if (format_version_ >= kFormatVersion2 && !historical_fields_.empty()) { auto it = historical_fields_.find(TransformKey{source_id, transform->ToString()}); if (it != historical_fields_.end()) { const auto& field = it->second; From 1ca6c612652b7f6d81e7a0c9ae82dc8beac97c9f Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Sat, 31 Jan 2026 12:49:56 +0800 Subject: [PATCH 2/3] fix: change format version back to 1/2/3 --- src/iceberg/constants.h | 5 -- src/iceberg/json_serde.cc | 27 +++++----- src/iceberg/manifest/manifest_writer.cc | 12 ++--- src/iceberg/table_metadata.cc | 14 +++--- src/iceberg/test/delete_file_index_test.cc | 16 +++--- src/iceberg/test/manifest_group_test.cc | 5 +- .../test/manifest_list_versions_test.cc | 26 +++++----- .../test/manifest_reader_stats_test.cc | 3 +- src/iceberg/test/manifest_reader_test.cc | 7 ++- .../test/manifest_writer_versions_test.cc | 49 ++++++++----------- .../test/rolling_manifest_writer_test.cc | 7 ++- src/iceberg/test/table_scan_test.cc | 17 +++---- src/iceberg/update/snapshot_update.cc | 4 +- src/iceberg/update/update_partition_spec.cc | 7 ++- 14 files changed, 86 insertions(+), 113 deletions(-) diff --git a/src/iceberg/constants.h b/src/iceberg/constants.h index 8c81ab60c..1d5941626 100644 --- a/src/iceberg/constants.h +++ b/src/iceberg/constants.h @@ -30,11 +30,6 @@ namespace iceberg { -/// \brief Iceberg table format versions -constexpr int8_t kFormatVersion1 = 1; -constexpr int8_t kFormatVersion2 = 2; -constexpr int8_t kFormatVersion3 = 3; - constexpr std::string_view kParquetFieldIdKey = "PARQUET:field_id"; constexpr int64_t kInvalidSnapshotId = -1; constexpr int64_t kInvalidSequenceNumber = -1; diff --git a/src/iceberg/json_serde.cc b/src/iceberg/json_serde.cc index e920f4e3a..004e8b3f5 100644 --- a/src/iceberg/json_serde.cc +++ b/src/iceberg/json_serde.cc @@ -838,7 +838,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) { json[kFormatVersion] = table_metadata.format_version; json[kTableUuid] = table_metadata.table_uuid; json[kLocation] = table_metadata.location; - if (table_metadata.format_version > kFormatVersion1) { + if (table_metadata.format_version > 1) { json[kLastSequenceNumber] = table_metadata.last_sequence_number; } json[kLastUpdatedMs] = UnixMsFromTimePointMs(table_metadata.last_updated_ms); @@ -847,7 +847,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) { // for older readers, continue writing the current schema as "schema". // this is only needed for v1 because support for schemas and current-schema-id // is required in v2 and later. - if (table_metadata.format_version == kFormatVersion1) { + if (table_metadata.format_version == 1) { for (const auto& schema : table_metadata.schemas) { if (schema->schema_id() == table_metadata.current_schema_id) { json[kSchema] = ToJson(*schema); @@ -861,7 +861,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) { json[kSchemas] = ToJsonList(table_metadata.schemas); // for older readers, continue writing the default spec as "partition-spec" - if (table_metadata.format_version == kFormatVersion1) { + if (table_metadata.format_version == 1) { for (const auto& partition_spec : table_metadata.partition_specs) { if (partition_spec->spec_id() == table_metadata.default_spec_id) { json[kPartitionSpec] = ToJson(*partition_spec); @@ -890,7 +890,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) { json[kCurrentSnapshotId] = nlohmann::json::value_t::null; } - if (table_metadata.format_version >= kFormatVersion3) { + if (table_metadata.format_version >= 3) { json[kNextRowId] = table_metadata.next_row_id; } @@ -945,7 +945,7 @@ Result> ParseSchemas( current_schema_id, SafeDumpJson(schema_array)); } } else { - if (format_version != kFormatVersion1) { + if (format_version != 1) { return JsonParseError("{} must exist in format v{}", kSchemas, format_version); } ICEBERG_ASSIGN_OR_RAISE(auto schema_json, @@ -983,7 +983,7 @@ Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version, partition_specs.push_back(std::move(spec)); } } else { - if (format_version != kFormatVersion1) { + if (format_version != 1) { return JsonParseError("{} must exist in format v{}", kPartitionSpecs, format_version); } @@ -999,9 +999,8 @@ Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version, std::vector fields; for (const auto& entry_json : partition_spec_json) { ICEBERG_ASSIGN_OR_RAISE( - auto field, - PartitionFieldFromJson( - entry_json, /*allow_field_id_missing=*/format_version == kFormatVersion1)); + auto field, PartitionFieldFromJson( + entry_json, /*allow_field_id_missing=*/format_version == 1)); int32_t field_id = field->field_id(); if (field_id == SchemaField::kInvalidFieldId) { // If the field ID is not set, we need to assign a new one @@ -1045,7 +1044,7 @@ Status ParseSortOrders(const nlohmann::json& json, int8_t format_version, sort_orders.push_back(std::move(sort_order)); } } else { - if (format_version > kFormatVersion1) { + if (format_version > 1) { return JsonParseError("{} must exist in format v{}", kSortOrders, format_version); } auto sort_order = SortOrder::Unsorted(); @@ -1067,7 +1066,7 @@ Result> TableMetadataFromJson(const nlohmann::jso ICEBERG_ASSIGN_OR_RAISE(table_metadata->format_version, GetJsonValue(json, kFormatVersion)); - if (table_metadata->format_version < kFormatVersion1 || + if (table_metadata->format_version < 1 || table_metadata->format_version > TableMetadata::kSupportedTableFormatVersion) { return JsonParseError("Cannot read unsupported version: {}", table_metadata->format_version); @@ -1078,7 +1077,7 @@ Result> TableMetadataFromJson(const nlohmann::jso ICEBERG_ASSIGN_OR_RAISE(table_metadata->location, GetJsonValue(json, kLocation)); - if (table_metadata->format_version > kFormatVersion1) { + if (table_metadata->format_version > 1) { ICEBERG_ASSIGN_OR_RAISE(table_metadata->last_sequence_number, GetJsonValue(json, kLastSequenceNumber)); } else { @@ -1100,7 +1099,7 @@ Result> TableMetadataFromJson(const nlohmann::jso ICEBERG_ASSIGN_OR_RAISE(table_metadata->last_partition_id, GetJsonValue(json, kLastPartitionId)); } else { - if (table_metadata->format_version > kFormatVersion1) { + if (table_metadata->format_version > 1) { return JsonParseError("{} must exist in format v{}", kLastPartitionId, table_metadata->format_version); } @@ -1130,7 +1129,7 @@ Result> TableMetadataFromJson(const nlohmann::jso table_metadata->current_snapshot_id, GetJsonValueOrDefault(json, kCurrentSnapshotId, kInvalidSnapshotId)); - if (table_metadata->format_version >= kFormatVersion3) { + if (table_metadata->format_version >= 3) { ICEBERG_ASSIGN_OR_RAISE(table_metadata->next_row_id, GetJsonValue(json, kNextRowId)); } else { diff --git a/src/iceberg/manifest/manifest_writer.cc b/src/iceberg/manifest/manifest_writer.cc index 55c6af128..fbe3d79bc 100644 --- a/src/iceberg/manifest/manifest_writer.cc +++ b/src/iceberg/manifest/manifest_writer.cc @@ -285,17 +285,17 @@ Result> ManifestWriter::MakeWriter( std::optional writer_first_row_id = std::nullopt; switch (format_version) { - case kFormatVersion1: { + case 1: { adapter = std::make_unique( snapshot_id, std::move(partition_spec), std::move(current_schema)); break; } - case kFormatVersion2: { + case 2: { adapter = std::make_unique( snapshot_id, std::move(partition_spec), std::move(current_schema), content); break; } - case kFormatVersion3: { + case 3: { adapter = std::make_unique( snapshot_id, first_row_id, std::move(partition_spec), std::move(current_schema), content); @@ -359,18 +359,18 @@ Result> ManifestListWriter::MakeWriter( std::unique_ptr adapter; switch (format_version) { - case kFormatVersion1: { + case 1: { adapter = std::make_unique(snapshot_id, parent_snapshot_id); break; } - case kFormatVersion2: { + case 2: { ICEBERG_PRECHECK(sequence_number.has_value(), "Sequence number is required for format version 2"); adapter = std::make_unique(snapshot_id, parent_snapshot_id, sequence_number.value()); break; } - case kFormatVersion3: { + case 3: { ICEBERG_PRECHECK(sequence_number.has_value(), "Sequence number is required for format version 3"); ICEBERG_PRECHECK(first_row_id.has_value(), diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index 97005ebdd..d3b5629c8 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -293,8 +293,7 @@ Result> TableMetadata::SnapshotById(int64_t snapshot_i } int64_t TableMetadata::NextSequenceNumber() const { - return format_version > kFormatVersion1 ? last_sequence_number + 1 - : kInitialSequenceNumber; + return format_version > 1 ? last_sequence_number + 1 : kInitialSequenceNumber; } namespace { @@ -857,10 +856,9 @@ Result TableMetadataBuilder::Impl::AddPartitionSpec(const PartitionSpec // Get current schema and validate the partition spec against it ICEBERG_ASSIGN_OR_RAISE(auto schema, metadata_.Schema()); ICEBERG_RETURN_UNEXPECTED(spec.Validate(*schema, /*allow_missing_fields=*/false)); - ICEBERG_CHECK(metadata_.format_version > kFormatVersion1 || - PartitionSpec::HasSequentialFieldIds(spec), - "Spec does not use sequential IDs that are required in v1: {}", - spec.ToString()); + ICEBERG_CHECK( + metadata_.format_version > 1 || PartitionSpec::HasSequentialFieldIds(spec), + "Spec does not use sequential IDs that are required in v1: {}", spec.ToString()); ICEBERG_ASSIGN_OR_RAISE( std::shared_ptr new_spec, @@ -1062,7 +1060,7 @@ Status TableMetadataBuilder::Impl::AddSnapshot(std::shared_ptr snapsho ICEBERG_CHECK(!snapshots_by_id_.contains(snapshot->snapshot_id), "Snapshot already exists for id: {}", snapshot->snapshot_id); ICEBERG_CHECK( - metadata_.format_version == kFormatVersion1 || + metadata_.format_version == 1 || snapshot->sequence_number > metadata_.last_sequence_number || !snapshot->parent_snapshot_id.has_value(), "Cannot add snapshot with sequence number {} older than last sequence number {}", @@ -1129,7 +1127,7 @@ Status TableMetadataBuilder::Impl::SetBranchSnapshotInternal(const Snapshot& sna } ICEBERG_CHECK( - metadata_.format_version == kFormatVersion1 || + metadata_.format_version == 1 || snapshot.sequence_number <= metadata_.last_sequence_number, "Last sequence number {} is less than existing snapshot sequence number {}", metadata_.last_sequence_number, snapshot.sequence_number); diff --git a/src/iceberg/test/delete_file_index_test.cc b/src/iceberg/test/delete_file_index_test.cc index 2c74d5023..b99a2816b 100644 --- a/src/iceberg/test/delete_file_index_test.cc +++ b/src/iceberg/test/delete_file_index_test.cc @@ -539,7 +539,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableWithUnrelatedPartitionDeletes) { TEST_P(DeleteFileIndexTest, TestPartitionedTableWithOlderPartitionDeletes) { auto version = GetParam(); - if (version >= kFormatVersion3) { + if (version >= 3) { GTEST_SKIP() << "DVs are not filtered using sequence numbers in V3+"; } @@ -569,7 +569,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableWithOlderPartitionDeletes) { TEST_P(DeleteFileIndexTest, TestPartitionedTableScanWithGlobalDeletes) { auto version = GetParam(); - if (version >= kFormatVersion3) { + if (version >= 3) { GTEST_SKIP() << "Different behavior for position deletes in V3"; } @@ -600,7 +600,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableScanWithGlobalDeletes) { TEST_P(DeleteFileIndexTest, TestPartitionedTableScanWithGlobalAndPartitionDeletes) { auto version = GetParam(); - if (version >= kFormatVersion3) { + if (version >= 3) { GTEST_SKIP() << "Different behavior for position deletes in V3"; } @@ -673,7 +673,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableSequenceNumbers) { TEST_P(DeleteFileIndexTest, TestUnpartitionedTableSequenceNumbers) { auto version = GetParam(); - if (version >= kFormatVersion3) { + if (version >= 3) { GTEST_SKIP() << "Different behavior in V3"; } @@ -842,7 +842,7 @@ TEST_P(DeleteFileIndexTest, TestEqualityDeletesGroup) { TEST_P(DeleteFileIndexTest, TestMixDeleteFilesAndDVs) { auto version = GetParam(); - if (version < kFormatVersion3) { + if (version < 3) { GTEST_SKIP() << "DVs only supported in V3+"; } @@ -898,7 +898,7 @@ TEST_P(DeleteFileIndexTest, TestMixDeleteFilesAndDVs) { TEST_P(DeleteFileIndexTest, TestMultipleDVs) { auto version = GetParam(); - if (version < kFormatVersion3) { + if (version < 3) { GTEST_SKIP() << "DVs only supported in V3+"; } @@ -924,7 +924,7 @@ TEST_P(DeleteFileIndexTest, TestMultipleDVs) { TEST_P(DeleteFileIndexTest, TestInvalidDVSequenceNumber) { auto version = GetParam(); - if (version < kFormatVersion3) { + if (version < 3) { GTEST_SKIP() << "DVs only supported in V3+"; } @@ -1177,6 +1177,6 @@ TEST_P(DeleteFileIndexTest, TestEqualityDeleteDiscardMetrics) { } INSTANTIATE_TEST_SUITE_P(DeleteFileIndexVersions, DeleteFileIndexTest, - testing::Values(kFormatVersion2, kFormatVersion3)); + testing::Values(2, 3)); } // namespace iceberg diff --git a/src/iceberg/test/manifest_group_test.cc b/src/iceberg/test/manifest_group_test.cc index 60698d449..34ff9993b 100644 --- a/src/iceberg/test/manifest_group_test.cc +++ b/src/iceberg/test/manifest_group_test.cc @@ -237,7 +237,7 @@ class ManifestGroupTest : public testing::TestWithParam { TEST_P(ManifestGroupTest, CreateAndGetEntries) { auto version = GetParam(); - if (version < kFormatVersion2) { + if (version < 2) { GTEST_SKIP() << "Delete files only supported in V2+"; } @@ -485,7 +485,6 @@ TEST_P(ManifestGroupTest, PartitionFilter) { } INSTANTIATE_TEST_SUITE_P(ManifestGroupVersions, ManifestGroupTest, - testing::Values(kFormatVersion1, kFormatVersion2, - kFormatVersion3)); + testing::Values(1, 2, 3)); } // namespace iceberg diff --git a/src/iceberg/test/manifest_list_versions_test.cc b/src/iceberg/test/manifest_list_versions_test.cc index 3b20fc69b..21218d339 100644 --- a/src/iceberg/test/manifest_list_versions_test.cc +++ b/src/iceberg/test/manifest_list_versions_test.cc @@ -190,9 +190,9 @@ class TestManifestListVersions : public ::testing::Test { TEST_F(TestManifestListVersions, TestV1WriteDeleteManifest) { const std::string manifest_list_path = CreateManifestListPath(); - ICEBERG_UNWRAP_OR_FAIL(auto writer, ManifestListWriter::MakeWriter( - kFormatVersion1, kSnapshotId, kSnapshotId - 1, - manifest_list_path, file_io_)); + ICEBERG_UNWRAP_OR_FAIL(auto writer, + ManifestListWriter::MakeWriter(1, kSnapshotId, kSnapshotId - 1, + manifest_list_path, file_io_)); auto status = writer->Add(kDeleteManifest); EXPECT_THAT(status, IsError(ErrorKind::kInvalidManifestList)); @@ -200,7 +200,7 @@ TEST_F(TestManifestListVersions, TestV1WriteDeleteManifest) { } TEST_F(TestManifestListVersions, TestV1Write) { - auto manifest = WriteAndReadManifestList(kFormatVersion1); + auto manifest = WriteAndReadManifestList(1); // V3 fields are not written and are defaulted EXPECT_FALSE(manifest.first_row_id.has_value()); @@ -224,7 +224,7 @@ TEST_F(TestManifestListVersions, TestV1Write) { } TEST_F(TestManifestListVersions, TestV2Write) { - auto manifest = WriteAndReadManifestList(kFormatVersion2); + auto manifest = WriteAndReadManifestList(2); // V3 fields are not written and are defaulted EXPECT_FALSE(manifest.first_row_id.has_value()); @@ -246,7 +246,7 @@ TEST_F(TestManifestListVersions, TestV2Write) { } TEST_F(TestManifestListVersions, TestV3Write) { - auto manifest = WriteAndReadManifestList(kFormatVersion3); + auto manifest = WriteAndReadManifestList(3); // All V3 fields should be read correctly EXPECT_EQ(manifest.manifest_path, kPath); @@ -272,7 +272,7 @@ TEST_F(TestManifestListVersions, TestV3WriteFirstRowIdAssignment) { constexpr int64_t kExpectedNextRowId = kSnapshotFirstRowId + kAddedRows + kExistingRows; auto manifest_list_path = - WriteManifestList(kFormatVersion3, kExpectedNextRowId, {missing_first_row_id}); + WriteManifestList(3, kExpectedNextRowId, {missing_first_row_id}); auto manifest = ReadManifestList(manifest_list_path); EXPECT_EQ(manifest.manifest_path, kPath); @@ -298,9 +298,8 @@ TEST_F(TestManifestListVersions, TestV3WriteMixedRowIdAssignment) { constexpr int64_t kExpectedNextRowId = kSnapshotFirstRowId + 2 * (kAddedRows + kExistingRows); - auto manifest_list_path = - WriteManifestList(kFormatVersion3, kExpectedNextRowId, - {missing_first_row_id, kTestManifest, missing_first_row_id}); + auto manifest_list_path = WriteManifestList( + 3, kExpectedNextRowId, {missing_first_row_id, kTestManifest, missing_first_row_id}); auto manifests = ReadAllManifests(manifest_list_path); EXPECT_EQ(manifests.size(), 3); @@ -330,7 +329,7 @@ TEST_F(TestManifestListVersions, TestV3WriteMixedRowIdAssignment) { TEST_F(TestManifestListVersions, TestV1ForwardCompatibility) { std::string manifest_list_path = - WriteManifestList(kFormatVersion1, kSnapshotFirstRowId, {kTestManifest}); + WriteManifestList(1, kSnapshotFirstRowId, {kTestManifest}); std::string expected_array_json = R"([ ["s3://bucket/table/m1.avro", 1024, 1, 987134631982734, 2, 343, 1, [], 5292, 857273, 22910, null] ])"; @@ -342,7 +341,7 @@ TEST_F(TestManifestListVersions, TestV2ForwardCompatibility) { // V2 manifest list files can be read by V1 readers, but the sequence numbers and // content will be ignored. std::string manifest_list_path = - WriteManifestList(kFormatVersion2, kSnapshotFirstRowId, {kTestManifest}); + WriteManifestList(2, kSnapshotFirstRowId, {kTestManifest}); std::string expected_array_json = R"([ ["s3://bucket/table/m1.avro", 1024, 1, 987134631982734, 2, 343, 1, [], 5292, 857273, 22910, null] ])"; @@ -445,8 +444,7 @@ TEST_F(TestManifestListVersions, TestManifestsPartitionSummary) { }; // Test for all format versions - for (int8_t format_version = kFormatVersion1; format_version <= kFormatVersion3; - ++format_version) { + for (int8_t format_version = 1; format_version <= 3; ++format_version) { int64_t expected_next_row_id = kSnapshotFirstRowId + manifest.added_rows_count.value() + manifest.existing_rows_count.value(); diff --git a/src/iceberg/test/manifest_reader_stats_test.cc b/src/iceberg/test/manifest_reader_stats_test.cc index ff7767e7a..a94dca120 100644 --- a/src/iceberg/test/manifest_reader_stats_test.cc +++ b/src/iceberg/test/manifest_reader_stats_test.cc @@ -217,7 +217,6 @@ TEST_P(TestManifestReaderStats, TestReadEntriesWithSelectCertainStatNotProjectSt } INSTANTIATE_TEST_SUITE_P(ManifestReaderStatsVersions, TestManifestReaderStats, - testing::Values(kFormatVersion1, kFormatVersion2, - kFormatVersion3)); + testing::Values(1, 2, 3)); } // namespace iceberg diff --git a/src/iceberg/test/manifest_reader_test.cc b/src/iceberg/test/manifest_reader_test.cc index 830e6951f..3e93f6ff5 100644 --- a/src/iceberg/test/manifest_reader_test.cc +++ b/src/iceberg/test/manifest_reader_test.cc @@ -255,7 +255,7 @@ TEST_P(TestManifestReader, TestManifestReaderWithPartitionMetadata) { TEST_P(TestManifestReader, TestDeleteFilesWithReferences) { auto version = GetParam(); - if (version < kFormatVersion2) { + if (version < 2) { GTEST_SKIP() << "Delete files only supported in V2+"; } @@ -294,7 +294,7 @@ TEST_P(TestManifestReader, TestDeleteFilesWithReferences) { TEST_P(TestManifestReader, TestDVs) { auto version = GetParam(); - if (version < kFormatVersion3) { + if (version < 3) { GTEST_SKIP() << "DVs only supported in V3+"; } @@ -419,7 +419,6 @@ TEST(ManifestReaderStaticTest, TestShouldDropStats) { } INSTANTIATE_TEST_SUITE_P(ManifestReaderVersions, TestManifestReader, - testing::Values(kFormatVersion1, kFormatVersion2, - kFormatVersion3)); + testing::Values(1, 2, 3)); } // namespace iceberg diff --git a/src/iceberg/test/manifest_writer_versions_test.cc b/src/iceberg/test/manifest_writer_versions_test.cc index 8ba0e6b8c..a3c42ae6c 100644 --- a/src/iceberg/test/manifest_writer_versions_test.cc +++ b/src/iceberg/test/manifest_writer_versions_test.cc @@ -411,7 +411,7 @@ class ManifestWriterVersionsTest : public ::testing::Test { }; TEST_F(ManifestWriterVersionsTest, TestV1Write) { - auto manifest = WriteManifest(kFormatVersion1, {data_file_}); + auto manifest = WriteManifest(1, {data_file_}); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -422,8 +422,8 @@ TEST_F(ManifestWriterVersionsTest, TestV1Write) { TEST_F(ManifestWriterVersionsTest, TestV1WriteDelete) { const std::string manifest_path = CreateManifestPath(); ICEBERG_UNWRAP_OR_FAIL( - auto writer, ManifestWriter::MakeWriter(kFormatVersion1, kSnapshotId, manifest_path, - file_io_, spec_, schema_)); + auto writer, ManifestWriter::MakeWriter(1, kSnapshotId, manifest_path, file_io_, + spec_, schema_)); ManifestEntry entry; entry.snapshot_id = kSnapshotId; @@ -436,8 +436,7 @@ TEST_F(ManifestWriterVersionsTest, TestV1WriteDelete) { } TEST_F(ManifestWriterVersionsTest, TestV1WriteWithInheritance) { - auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion1, {data_file_})}, - kFormatVersion1); + auto manifests = WriteAndReadManifests({WriteManifest(1, {data_file_})}, 1); ASSERT_EQ(manifests.size(), 1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); @@ -448,7 +447,7 @@ TEST_F(ManifestWriterVersionsTest, TestV1WriteWithInheritance) { } TEST_F(ManifestWriterVersionsTest, TestV2Write) { - auto manifest = WriteManifest(kFormatVersion2, {data_file_}); + auto manifest = WriteManifest(2, {data_file_}); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -458,8 +457,7 @@ TEST_F(ManifestWriterVersionsTest, TestV2Write) { } TEST_F(ManifestWriterVersionsTest, TestV2WriteWithInheritance) { - auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion2, {data_file_})}, - kFormatVersion2); + auto manifests = WriteAndReadManifests({WriteManifest(2, {data_file_})}, 2); CheckManifest(manifests[0], kSequenceNumber, kSequenceNumber); auto entries = ReadManifest(manifests[0]); ASSERT_EQ(entries.size(), 1); @@ -468,7 +466,7 @@ TEST_F(ManifestWriterVersionsTest, TestV2WriteWithInheritance) { } TEST_F(ManifestWriterVersionsTest, TestV2PlusWriteDeleteV2) { - auto manifest = WriteDeleteManifest(kFormatVersion2, delete_file_); + auto manifest = WriteDeleteManifest(2, delete_file_); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -479,13 +477,12 @@ TEST_F(ManifestWriterVersionsTest, TestV2PlusWriteDeleteV2) { TEST_F(ManifestWriterVersionsTest, TestV2ManifestListRewriteWithInheritance) { // write with v1 - auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion1, {data_file_})}, - kFormatVersion1); + auto manifests = WriteAndReadManifests({WriteManifest(1, {data_file_})}, 1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite existing metadata with v2 manifest list - auto manifests2 = WriteAndReadManifests(manifests, kFormatVersion2); + auto manifests2 = WriteAndReadManifests(manifests, 2); // the ManifestFile did not change and should still have its original sequence number, 0 CheckManifest(manifests2[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); @@ -498,18 +495,17 @@ TEST_F(ManifestWriterVersionsTest, TestV2ManifestListRewriteWithInheritance) { TEST_F(ManifestWriterVersionsTest, TestV2ManifestRewriteWithInheritance) { // write with v1 - auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion1, {data_file_})}, - kFormatVersion1); + auto manifests = WriteAndReadManifests({WriteManifest(1, {data_file_})}, 1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite the manifest file using a v2 manifest - auto rewritten_manifest = RewriteManifest(manifests[0], kFormatVersion2); + auto rewritten_manifest = RewriteManifest(manifests[0], 2); CheckRewrittenManifest(rewritten_manifest, kInvalidSequenceNumber, TableMetadata::kInitialSequenceNumber); // add the v2 manifest to a v2 manifest list, with a sequence number - auto manifests2 = WriteAndReadManifests({rewritten_manifest}, kFormatVersion2); + auto manifests2 = WriteAndReadManifests({rewritten_manifest}, 2); // the ManifestFile is new so it has a sequence number, but the min sequence number 0 is // from the entry CheckRewrittenManifest(manifests2[0], kSequenceNumber, @@ -522,7 +518,7 @@ TEST_F(ManifestWriterVersionsTest, TestV2ManifestRewriteWithInheritance) { } TEST_F(ManifestWriterVersionsTest, TestV3Write) { - auto manifest = WriteManifest(kFormatVersion3, {data_file_}); + auto manifest = WriteManifest(3, {data_file_}); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -532,9 +528,8 @@ TEST_F(ManifestWriterVersionsTest, TestV3Write) { } TEST_F(ManifestWriterVersionsTest, TestV3WriteWithInheritance) { - auto manifests = WriteAndReadManifests( - {WriteManifest(kFormatVersion3, {data_file_without_first_row_id_})}, - kFormatVersion3); + auto manifests = + WriteAndReadManifests({WriteManifest(3, {data_file_without_first_row_id_})}, 3); CheckManifest(manifests[0], kSequenceNumber, kSequenceNumber); ASSERT_EQ(manifests[0].content, ManifestContent::kData); @@ -549,7 +544,7 @@ TEST_F(ManifestWriterVersionsTest, TestV3WriteWithInheritance) { TEST_F(ManifestWriterVersionsTest, TestV3WriteFirstRowIdAssignment) { auto manifests = WriteAndReadManifests( - {WriteManifest(kFormatVersion3, + {WriteManifest(3, {data_file_without_first_row_id_, data_file_without_first_row_id_})}, 3); ASSERT_EQ(manifests[0].content, ManifestContent::kData); @@ -568,13 +563,12 @@ TEST_F(ManifestWriterVersionsTest, TestV3WriteFirstRowIdAssignment) { TEST_F(ManifestWriterVersionsTest, TestV3ManifestListRewriteWithInheritance) { // write with v1 - auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion1, {data_file_})}, - kFormatVersion1); + auto manifests = WriteAndReadManifests({WriteManifest(1, {data_file_})}, 1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite existing metadata with a manifest list - auto manifests3 = WriteAndReadManifests(manifests, kFormatVersion3); + auto manifests3 = WriteAndReadManifests(manifests, 3); // the ManifestFile did not change and should still have its original sequence number, 0 CheckManifest(manifests3[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); @@ -588,18 +582,17 @@ TEST_F(ManifestWriterVersionsTest, TestV3ManifestListRewriteWithInheritance) { TEST_F(ManifestWriterVersionsTest, TestV3ManifestRewriteWithInheritance) { // write with v1 - auto manifests = WriteAndReadManifests({WriteManifest(kFormatVersion1, {data_file_})}, - kFormatVersion1); + auto manifests = WriteAndReadManifests({WriteManifest(1, {data_file_})}, 1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite the manifest file using a v3 manifest - auto rewritten_manifest = RewriteManifest(manifests[0], kFormatVersion3); + auto rewritten_manifest = RewriteManifest(manifests[0], 3); CheckRewrittenManifest(rewritten_manifest, kInvalidSequenceNumber, TableMetadata::kInitialSequenceNumber); // add the v3 manifest to a v3 manifest list, with a sequence number - auto manifests3 = WriteAndReadManifests({rewritten_manifest}, kFormatVersion3); + auto manifests3 = WriteAndReadManifests({rewritten_manifest}, 3); // the ManifestFile is new so it has a sequence number, but the min sequence number 0 is // from the entry CheckRewrittenManifest(manifests3[0], kSequenceNumber, diff --git a/src/iceberg/test/rolling_manifest_writer_test.cc b/src/iceberg/test/rolling_manifest_writer_test.cc index 0d3a93dcc..b996eb166 100644 --- a/src/iceberg/test/rolling_manifest_writer_test.cc +++ b/src/iceberg/test/rolling_manifest_writer_test.cc @@ -171,7 +171,7 @@ TEST_P(RollingManifestWriterTest, TestRollingManifestWriterNoRecords) { TEST_P(RollingManifestWriterTest, TestRollingDeleteManifestWriterNoRecords) { auto format_version = GetParam(); - if (format_version < kFormatVersion2) { + if (format_version < 2) { GTEST_SKIP() << "Delete manifests only supported in V2+"; } RollingManifestWriter writer(NewRollingWriteDeleteManifestFactory(format_version), @@ -240,7 +240,7 @@ TEST_P(RollingManifestWriterTest, TestRollingManifestWriterSplitFiles) { TEST_P(RollingManifestWriterTest, TestRollingDeleteManifestWriterSplitFiles) { auto format_version = GetParam(); - if (format_version < kFormatVersion2) { + if (format_version < 2) { GTEST_SKIP() << "Delete manifests only supported in V2+"; } RollingManifestWriter writer(NewRollingWriteDeleteManifestFactory(format_version), @@ -293,7 +293,6 @@ TEST_P(RollingManifestWriterTest, TestRollingDeleteManifestWriterSplitFiles) { } INSTANTIATE_TEST_SUITE_P(TestRollingManifestWriter, RollingManifestWriterTest, - ::testing::Values(kFormatVersion1, kFormatVersion2, - kFormatVersion3)); + ::testing::Values(1, 2, 3)); } // namespace iceberg diff --git a/src/iceberg/test/table_scan_test.cc b/src/iceberg/test/table_scan_test.cc index e669ede64..2eed86732 100644 --- a/src/iceberg/test/table_scan_test.cc +++ b/src/iceberg/test/table_scan_test.cc @@ -182,8 +182,7 @@ class TableScanTest : public testing::TestWithParam { auto writer_result = ManifestWriter::MakeWriter( format_version, snapshot_id, manifest_path, file_io_, spec, schema_, ManifestContent::kData, - /*first_row_id=*/format_version >= kFormatVersion3 ? std::optional(0L) - : std::nullopt); + /*first_row_id=*/format_version >= 3 ? std::optional(0L) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -233,11 +232,9 @@ class TableScanTest : public testing::TestWithParam { auto writer_result = ManifestListWriter::MakeWriter( format_version, snapshot_id, kParentSnapshotId, manifest_list_path, file_io_, - /*sequence_number=*/format_version >= kFormatVersion2 - ? std::optional(sequence_number) - : std::nullopt, - /*first_row_id=*/format_version >= kFormatVersion3 ? std::optional(0L) - : std::nullopt); + /*sequence_number=*/format_version >= 2 ? std::optional(sequence_number) + : std::nullopt, + /*first_row_id=*/format_version >= 3 ? std::optional(0L) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -591,7 +588,7 @@ TEST_P(TableScanTest, PlanFilesWithFilter) { TEST_P(TableScanTest, PlanFilesWithDeleteFiles) { auto version = GetParam(); - if (version < kFormatVersion2) { + if (version < 2) { GTEST_SKIP() << "Delete files only supported in V2+"; } @@ -669,8 +666,6 @@ TEST_P(TableScanTest, PlanFilesWithDeleteFiles) { } } -INSTANTIATE_TEST_SUITE_P(TableScanVersions, TableScanTest, - testing::Values(kFormatVersion1, kFormatVersion2, - kFormatVersion3)); +INSTANTIATE_TEST_SUITE_P(TableScanVersions, TableScanTest, testing::Values(1, 2, 3)); } // namespace iceberg diff --git a/src/iceberg/update/snapshot_update.cc b/src/iceberg/update/snapshot_update.cc index 1b3ff7839..38c5129f4 100644 --- a/src/iceberg/update/snapshot_update.cc +++ b/src/iceberg/update/snapshot_update.cc @@ -157,7 +157,7 @@ SnapshotUpdate::~SnapshotUpdate() = default; SnapshotUpdate::SnapshotUpdate(std::shared_ptr transaction) : PendingUpdate(std::move(transaction)), can_inherit_snapshot_id_( - base().format_version > kFormatVersion1 || + base().format_version > 1 || base().properties.Get(TableProperties::kSnapshotIdInheritanceEnabled)), commit_uuid_(Uuid::GenerateV7().ToString()), target_manifest_size_bytes_( @@ -261,7 +261,7 @@ Result SnapshotUpdate::Apply() { std::optional next_row_id; std::optional assigned_rows; - if (base().format_version >= kFormatVersion3) { + if (base().format_version >= 3) { ICEBERG_CHECK(writer->next_row_id().has_value(), "row id is required by format version >= 3"); next_row_id = base().next_row_id; diff --git a/src/iceberg/update/update_partition_spec.cc b/src/iceberg/update/update_partition_spec.cc index ecb22296c..812540f2b 100644 --- a/src/iceberg/update/update_partition_spec.cc +++ b/src/iceberg/update/update_partition_spec.cc @@ -25,7 +25,6 @@ #include #include -#include "iceberg/constants.h" #include "iceberg/expression/term.h" #include "iceberg/partition_field.h" #include "iceberg/partition_spec.h" @@ -81,7 +80,7 @@ UpdatePartitionSpec::UpdatePartitionSpec(std::shared_ptr transactio } // Build index of historical partition fields for efficient recycling (V2+) - if (format_version_ >= kFormatVersion2) { + if (format_version_ >= 2) { BuildHistoricalFieldsIndex(); } } @@ -326,7 +325,7 @@ Result UpdatePartitionSpec::Apply() { } else { new_fields.push_back(field); } - } else if (format_version_ < kFormatVersion2) { + } else if (format_version_ < 2) { // field IDs were not required for v1 and were assigned sequentially in each // partition spec starting at 1,000. // To maintain consistent field ids across partition specs in v1 tables, any @@ -358,7 +357,7 @@ int32_t UpdatePartitionSpec::AssignFieldId() { return ++last_assigned_partition_ PartitionField UpdatePartitionSpec::RecycleOrCreatePartitionField( int32_t source_id, std::shared_ptr transform, std::string_view name) { // In V2+, use pre-built index for O(1) lookup instead of O(n*m) iteration - if (format_version_ >= kFormatVersion2 && !historical_fields_.empty()) { + if (format_version_ >= 2 && !historical_fields_.empty()) { auto it = historical_fields_.find(TransformKey{source_id, transform->ToString()}); if (it != historical_fields_.end()) { const auto& field = it->second; From 7c43d2a3196dc303caa34a7a1763f678b216071a Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Sat, 31 Jan 2026 13:03:48 +0800 Subject: [PATCH 3/3] fix: /*format_version=*/ param comments --- .../test/manifest_list_versions_test.cc | 21 ++++---- .../test/manifest_writer_versions_test.cc | 48 +++++++++++-------- 2 files changed, 38 insertions(+), 31 deletions(-) diff --git a/src/iceberg/test/manifest_list_versions_test.cc b/src/iceberg/test/manifest_list_versions_test.cc index 21218d339..9c16a02ec 100644 --- a/src/iceberg/test/manifest_list_versions_test.cc +++ b/src/iceberg/test/manifest_list_versions_test.cc @@ -190,9 +190,9 @@ class TestManifestListVersions : public ::testing::Test { TEST_F(TestManifestListVersions, TestV1WriteDeleteManifest) { const std::string manifest_list_path = CreateManifestListPath(); - ICEBERG_UNWRAP_OR_FAIL(auto writer, - ManifestListWriter::MakeWriter(1, kSnapshotId, kSnapshotId - 1, - manifest_list_path, file_io_)); + ICEBERG_UNWRAP_OR_FAIL(auto writer, ManifestListWriter::MakeWriter( + /*format_version=*/1, kSnapshotId, + kSnapshotId - 1, manifest_list_path, file_io_)); auto status = writer->Add(kDeleteManifest); EXPECT_THAT(status, IsError(ErrorKind::kInvalidManifestList)); @@ -200,7 +200,7 @@ TEST_F(TestManifestListVersions, TestV1WriteDeleteManifest) { } TEST_F(TestManifestListVersions, TestV1Write) { - auto manifest = WriteAndReadManifestList(1); + auto manifest = WriteAndReadManifestList(/*format_version=*/1); // V3 fields are not written and are defaulted EXPECT_FALSE(manifest.first_row_id.has_value()); @@ -224,7 +224,7 @@ TEST_F(TestManifestListVersions, TestV1Write) { } TEST_F(TestManifestListVersions, TestV2Write) { - auto manifest = WriteAndReadManifestList(2); + auto manifest = WriteAndReadManifestList(/*format_version=*/2); // V3 fields are not written and are defaulted EXPECT_FALSE(manifest.first_row_id.has_value()); @@ -246,7 +246,7 @@ TEST_F(TestManifestListVersions, TestV2Write) { } TEST_F(TestManifestListVersions, TestV3Write) { - auto manifest = WriteAndReadManifestList(3); + auto manifest = WriteAndReadManifestList(/*format_version=*/3); // All V3 fields should be read correctly EXPECT_EQ(manifest.manifest_path, kPath); @@ -272,7 +272,7 @@ TEST_F(TestManifestListVersions, TestV3WriteFirstRowIdAssignment) { constexpr int64_t kExpectedNextRowId = kSnapshotFirstRowId + kAddedRows + kExistingRows; auto manifest_list_path = - WriteManifestList(3, kExpectedNextRowId, {missing_first_row_id}); + WriteManifestList(/*format_version=*/3, kExpectedNextRowId, {missing_first_row_id}); auto manifest = ReadManifestList(manifest_list_path); EXPECT_EQ(manifest.manifest_path, kPath); @@ -299,7 +299,8 @@ TEST_F(TestManifestListVersions, TestV3WriteMixedRowIdAssignment) { kSnapshotFirstRowId + 2 * (kAddedRows + kExistingRows); auto manifest_list_path = WriteManifestList( - 3, kExpectedNextRowId, {missing_first_row_id, kTestManifest, missing_first_row_id}); + /*format_version=*/3, kExpectedNextRowId, + {missing_first_row_id, kTestManifest, missing_first_row_id}); auto manifests = ReadAllManifests(manifest_list_path); EXPECT_EQ(manifests.size(), 3); @@ -329,7 +330,7 @@ TEST_F(TestManifestListVersions, TestV3WriteMixedRowIdAssignment) { TEST_F(TestManifestListVersions, TestV1ForwardCompatibility) { std::string manifest_list_path = - WriteManifestList(1, kSnapshotFirstRowId, {kTestManifest}); + WriteManifestList(/*format_version=*/1, kSnapshotFirstRowId, {kTestManifest}); std::string expected_array_json = R"([ ["s3://bucket/table/m1.avro", 1024, 1, 987134631982734, 2, 343, 1, [], 5292, 857273, 22910, null] ])"; @@ -341,7 +342,7 @@ TEST_F(TestManifestListVersions, TestV2ForwardCompatibility) { // V2 manifest list files can be read by V1 readers, but the sequence numbers and // content will be ignored. std::string manifest_list_path = - WriteManifestList(2, kSnapshotFirstRowId, {kTestManifest}); + WriteManifestList(/*format_version=*/2, kSnapshotFirstRowId, {kTestManifest}); std::string expected_array_json = R"([ ["s3://bucket/table/m1.avro", 1024, 1, 987134631982734, 2, 343, 1, [], 5292, 857273, 22910, null] ])"; diff --git a/src/iceberg/test/manifest_writer_versions_test.cc b/src/iceberg/test/manifest_writer_versions_test.cc index a3c42ae6c..fc61980a7 100644 --- a/src/iceberg/test/manifest_writer_versions_test.cc +++ b/src/iceberg/test/manifest_writer_versions_test.cc @@ -411,7 +411,7 @@ class ManifestWriterVersionsTest : public ::testing::Test { }; TEST_F(ManifestWriterVersionsTest, TestV1Write) { - auto manifest = WriteManifest(1, {data_file_}); + auto manifest = WriteManifest(/*format_version=*/1, {data_file_}); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -422,8 +422,8 @@ TEST_F(ManifestWriterVersionsTest, TestV1Write) { TEST_F(ManifestWriterVersionsTest, TestV1WriteDelete) { const std::string manifest_path = CreateManifestPath(); ICEBERG_UNWRAP_OR_FAIL( - auto writer, ManifestWriter::MakeWriter(1, kSnapshotId, manifest_path, file_io_, - spec_, schema_)); + auto writer, ManifestWriter::MakeWriter(/*format_version=*/1, kSnapshotId, + manifest_path, file_io_, spec_, schema_)); ManifestEntry entry; entry.snapshot_id = kSnapshotId; @@ -436,7 +436,8 @@ TEST_F(ManifestWriterVersionsTest, TestV1WriteDelete) { } TEST_F(ManifestWriterVersionsTest, TestV1WriteWithInheritance) { - auto manifests = WriteAndReadManifests({WriteManifest(1, {data_file_})}, 1); + auto manifests = + WriteAndReadManifests({WriteManifest(/*format_version=*/1, {data_file_})}, 1); ASSERT_EQ(manifests.size(), 1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); @@ -447,7 +448,7 @@ TEST_F(ManifestWriterVersionsTest, TestV1WriteWithInheritance) { } TEST_F(ManifestWriterVersionsTest, TestV2Write) { - auto manifest = WriteManifest(2, {data_file_}); + auto manifest = WriteManifest(/*format_version=*/2, {data_file_}); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -457,7 +458,8 @@ TEST_F(ManifestWriterVersionsTest, TestV2Write) { } TEST_F(ManifestWriterVersionsTest, TestV2WriteWithInheritance) { - auto manifests = WriteAndReadManifests({WriteManifest(2, {data_file_})}, 2); + auto manifests = + WriteAndReadManifests({WriteManifest(/*format_version=*/2, {data_file_})}, 2); CheckManifest(manifests[0], kSequenceNumber, kSequenceNumber); auto entries = ReadManifest(manifests[0]); ASSERT_EQ(entries.size(), 1); @@ -466,7 +468,7 @@ TEST_F(ManifestWriterVersionsTest, TestV2WriteWithInheritance) { } TEST_F(ManifestWriterVersionsTest, TestV2PlusWriteDeleteV2) { - auto manifest = WriteDeleteManifest(2, delete_file_); + auto manifest = WriteDeleteManifest(/*format_version=*/2, delete_file_); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -477,12 +479,13 @@ TEST_F(ManifestWriterVersionsTest, TestV2PlusWriteDeleteV2) { TEST_F(ManifestWriterVersionsTest, TestV2ManifestListRewriteWithInheritance) { // write with v1 - auto manifests = WriteAndReadManifests({WriteManifest(1, {data_file_})}, 1); + auto manifests = + WriteAndReadManifests({WriteManifest(/*format_version=*/1, {data_file_})}, 1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite existing metadata with v2 manifest list - auto manifests2 = WriteAndReadManifests(manifests, 2); + auto manifests2 = WriteAndReadManifests(manifests, /*format_version=*/2); // the ManifestFile did not change and should still have its original sequence number, 0 CheckManifest(manifests2[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); @@ -495,17 +498,18 @@ TEST_F(ManifestWriterVersionsTest, TestV2ManifestListRewriteWithInheritance) { TEST_F(ManifestWriterVersionsTest, TestV2ManifestRewriteWithInheritance) { // write with v1 - auto manifests = WriteAndReadManifests({WriteManifest(1, {data_file_})}, 1); + auto manifests = + WriteAndReadManifests({WriteManifest(/*format_version=*/1, {data_file_})}, 1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite the manifest file using a v2 manifest - auto rewritten_manifest = RewriteManifest(manifests[0], 2); + auto rewritten_manifest = RewriteManifest(manifests[0], /*format_version=*/2); CheckRewrittenManifest(rewritten_manifest, kInvalidSequenceNumber, TableMetadata::kInitialSequenceNumber); // add the v2 manifest to a v2 manifest list, with a sequence number - auto manifests2 = WriteAndReadManifests({rewritten_manifest}, 2); + auto manifests2 = WriteAndReadManifests({rewritten_manifest}, /*format_version=*/2); // the ManifestFile is new so it has a sequence number, but the min sequence number 0 is // from the entry CheckRewrittenManifest(manifests2[0], kSequenceNumber, @@ -518,7 +522,7 @@ TEST_F(ManifestWriterVersionsTest, TestV2ManifestRewriteWithInheritance) { } TEST_F(ManifestWriterVersionsTest, TestV3Write) { - auto manifest = WriteManifest(3, {data_file_}); + auto manifest = WriteManifest(/*format_version=*/3, {data_file_}); CheckManifest(manifest, kInvalidSequenceNumber, kInvalidSequenceNumber); auto entries = ReadManifest(manifest); ASSERT_EQ(entries.size(), 1); @@ -528,8 +532,8 @@ TEST_F(ManifestWriterVersionsTest, TestV3Write) { } TEST_F(ManifestWriterVersionsTest, TestV3WriteWithInheritance) { - auto manifests = - WriteAndReadManifests({WriteManifest(3, {data_file_without_first_row_id_})}, 3); + auto manifests = WriteAndReadManifests( + {WriteManifest(/*format_version=*/3, {data_file_without_first_row_id_})}, 3); CheckManifest(manifests[0], kSequenceNumber, kSequenceNumber); ASSERT_EQ(manifests[0].content, ManifestContent::kData); @@ -544,7 +548,7 @@ TEST_F(ManifestWriterVersionsTest, TestV3WriteWithInheritance) { TEST_F(ManifestWriterVersionsTest, TestV3WriteFirstRowIdAssignment) { auto manifests = WriteAndReadManifests( - {WriteManifest(3, + {WriteManifest(/*format_version=*/3, {data_file_without_first_row_id_, data_file_without_first_row_id_})}, 3); ASSERT_EQ(manifests[0].content, ManifestContent::kData); @@ -563,12 +567,13 @@ TEST_F(ManifestWriterVersionsTest, TestV3WriteFirstRowIdAssignment) { TEST_F(ManifestWriterVersionsTest, TestV3ManifestListRewriteWithInheritance) { // write with v1 - auto manifests = WriteAndReadManifests({WriteManifest(1, {data_file_})}, 1); + auto manifests = + WriteAndReadManifests({WriteManifest(/*format_version=*/1, {data_file_})}, 1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite existing metadata with a manifest list - auto manifests3 = WriteAndReadManifests(manifests, 3); + auto manifests3 = WriteAndReadManifests(manifests, /*format_version=*/3); // the ManifestFile did not change and should still have its original sequence number, 0 CheckManifest(manifests3[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); @@ -582,17 +587,18 @@ TEST_F(ManifestWriterVersionsTest, TestV3ManifestListRewriteWithInheritance) { TEST_F(ManifestWriterVersionsTest, TestV3ManifestRewriteWithInheritance) { // write with v1 - auto manifests = WriteAndReadManifests({WriteManifest(1, {data_file_})}, 1); + auto manifests = + WriteAndReadManifests({WriteManifest(/*format_version=*/1, {data_file_})}, 1); CheckManifest(manifests[0], TableMetadata::kInitialSequenceNumber, TableMetadata::kInitialSequenceNumber); // rewrite the manifest file using a v3 manifest - auto rewritten_manifest = RewriteManifest(manifests[0], 3); + auto rewritten_manifest = RewriteManifest(manifests[0], /*format_version=*/3); CheckRewrittenManifest(rewritten_manifest, kInvalidSequenceNumber, TableMetadata::kInitialSequenceNumber); // add the v3 manifest to a v3 manifest list, with a sequence number - auto manifests3 = WriteAndReadManifests({rewritten_manifest}, 3); + auto manifests3 = WriteAndReadManifests({rewritten_manifest}, /*format_version=*/3); // the ManifestFile is new so it has a sequence number, but the min sequence number 0 is // from the entry CheckRewrittenManifest(manifests3[0], kSequenceNumber,