From 73e71b815f3004ec9df10c4b591116b51d01ee05 Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Wed, 7 Jan 2026 13:18:09 +0800 Subject: [PATCH 1/2] refactor: Add factory functions for ManifestWriter and ManifestListWriter - Add ManifestWriter::MakeWriter() factory function that takes format_version parameter and routes to appropriate MakeV1Writer/MakeV2Writer/MakeV3Writer - Add ManifestListWriter::MakeWriter() factory function with same pattern - Update test cases to use the new factory functions instead of scattered if-else chains checking format_version - Centralizes version handling logic, making it easier to add v4, v5, etc. in the future --- src/iceberg/manifest/manifest_writer.cc | 52 ++++++++++++ src/iceberg/manifest/manifest_writer.h | 38 +++++++++ src/iceberg/test/delete_file_index_test.cc | 15 +--- src/iceberg/test/manifest_group_test.cc | 55 +++---------- .../test/manifest_list_versions_test.cc | 27 ++----- .../test/manifest_reader_stats_test.cc | 24 +----- src/iceberg/test/manifest_reader_test.cc | 38 ++------- .../test/manifest_writer_versions_test.cc | 81 +++++-------------- .../test/rolling_manifest_writer_test.cc | 38 ++------- 9 files changed, 153 insertions(+), 215 deletions(-) diff --git a/src/iceberg/manifest/manifest_writer.cc b/src/iceberg/manifest/manifest_writer.cc index 649797fee..0899869ad 100644 --- a/src/iceberg/manifest/manifest_writer.cc +++ b/src/iceberg/manifest/manifest_writer.cc @@ -365,6 +365,32 @@ Result> ManifestWriter::MakeV3Writer( 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, + std::optional 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: + ICEBERG_PRECHECK(content.has_value(), + "ManifestContent is required for format version 2"); + return MakeV2Writer(snapshot_id, manifest_location, std::move(file_io), + std::move(partition_spec), std::move(current_schema), + content.value()); + case 3: + ICEBERG_PRECHECK(content.has_value(), + "ManifestContent is required for format version 3"); + return MakeV3Writer(snapshot_id, first_row_id, manifest_location, + std::move(file_io), std::move(partition_spec), + std::move(current_schema), content.value()); + default: + return NotSupported("Format version {} is not supported", format_version); + } +} + ManifestListWriter::ManifestListWriter(std::unique_ptr writer, std::unique_ptr adapter) : writer_(std::move(writer)), adapter_(std::move(adapter)) {} @@ -452,4 +478,30 @@ Result> ManifestListWriter::MakeV3Writer( 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) { + switch (format_version) { + case 1: + return MakeV1Writer(snapshot_id, parent_snapshot_id, manifest_list_location, + std::move(file_io)); + case 2: + 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: + 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)); + default: + return NotSupported("Format version {} is not supported", format_version); + } +} + } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_writer.h b/src/iceberg/manifest/manifest_writer.h index 6f468240d..5a095b28b 100644 --- a/src/iceberg/manifest/manifest_writer.h +++ b/src/iceberg/manifest/manifest_writer.h @@ -158,6 +158,26 @@ class ICEBERG_EXPORT ManifestWriter { 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.). + /// \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 (required for format_version >= 2). + /// \param first_row_id First row ID of the snapshot (required for format_version >= 3). + /// \return A Result containing the writer or an error. + static Result> 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, + std::optional content = std::nullopt, + std::optional first_row_id = std::nullopt); + private: // Private constructor for internal use only, use the static Make*Writer methods // instead. @@ -240,6 +260,24 @@ class ICEBERG_EXPORT ManifestListWriter { 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.). + /// \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. + /// \param sequence_number Sequence number of the snapshot (required for format_version + /// >= 2). + /// \param first_row_id First row ID of the snapshot (required for format_version >= 3). + /// \return A Result containing the writer or an error. + static Result> 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::nullopt, + std::optional first_row_id = std::nullopt); + private: // Private constructor for internal use only, use the static Make*Writer methods // instead. diff --git a/src/iceberg/test/delete_file_index_test.cc b/src/iceberg/test/delete_file_index_test.cc index cf75fe2dc..10895e258 100644 --- a/src/iceberg/test/delete_file_index_test.cc +++ b/src/iceberg/test/delete_file_index_test.cc @@ -165,17 +165,10 @@ class DeleteFileIndexTest : public testing::TestWithParam { std::shared_ptr spec) { const std::string manifest_path = MakeManifestPath(); - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 2) { - writer_result = ManifestWriter::MakeV2Writer( - snapshot_id, manifest_path, file_io_, spec, schema_, ManifestContent::kDeletes); - } else if (format_version == 3) { - writer_result = ManifestWriter::MakeV3Writer( - snapshot_id, /*first_row_id=*/std::nullopt, manifest_path, file_io_, spec, - schema_, ManifestContent::kDeletes); - } + auto writer_result = ManifestWriter::MakeWriter( + format_version, snapshot_id, manifest_path, file_io_, spec, schema_, + ManifestContent::kDeletes, + format_version >= 3 ? std::optional(std::nullopt) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); diff --git a/src/iceberg/test/manifest_group_test.cc b/src/iceberg/test/manifest_group_test.cc index c7655342b..8c68de1b1 100644 --- a/src/iceberg/test/manifest_group_test.cc +++ b/src/iceberg/test/manifest_group_test.cc @@ -135,21 +135,10 @@ class ManifestGroupTest : public testing::TestWithParam { std::shared_ptr spec) { const std::string manifest_path = MakeManifestPath(); - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 1) { - writer_result = ManifestWriter::MakeV1Writer(snapshot_id, manifest_path, file_io_, - spec, schema_); - } else if (format_version == 2) { - writer_result = ManifestWriter::MakeV2Writer(snapshot_id, manifest_path, file_io_, - spec, schema_, ManifestContent::kData); - } else if (format_version == 3) { - writer_result = - ManifestWriter::MakeV3Writer(snapshot_id, /*first_row_id=*/0L, manifest_path, - file_io_, spec, schema_, ManifestContent::kData); - } - + 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); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -168,18 +157,11 @@ class ManifestGroupTest : public testing::TestWithParam { std::shared_ptr spec) { const std::string manifest_path = MakeManifestPath(); - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 2) { - writer_result = ManifestWriter::MakeV2Writer( - snapshot_id, manifest_path, file_io_, spec, schema_, ManifestContent::kDeletes); - } else if (format_version == 3) { - writer_result = ManifestWriter::MakeV3Writer( - snapshot_id, /*first_row_id=*/std::nullopt, manifest_path, file_io_, spec, - schema_, ManifestContent::kDeletes); - } - + auto writer_result = ManifestWriter::MakeWriter( + format_version, snapshot_id, manifest_path, file_io_, spec, schema_, + ManifestContent::kDeletes, + /*first_row_id=*/format_version >= 3 ? std::optional(std::nullopt) + : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -213,21 +195,10 @@ class ManifestGroupTest : public testing::TestWithParam { constexpr int64_t kParentSnapshotId = 0L; constexpr int64_t kSnapshotFirstRowId = 0L; - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 1) { - writer_result = ManifestListWriter::MakeV1Writer(snapshot_id, kParentSnapshotId, - manifest_list_path, file_io_); - } else if (format_version == 2) { - writer_result = ManifestListWriter::MakeV2Writer( - snapshot_id, kParentSnapshotId, sequence_number, manifest_list_path, file_io_); - } else if (format_version == 3) { - writer_result = ManifestListWriter::MakeV3Writer( - snapshot_id, kParentSnapshotId, sequence_number, kSnapshotFirstRowId, - manifest_list_path, file_io_); - } - + auto writer_result = ManifestListWriter::MakeWriter( + format_version, snapshot_id, kParentSnapshotId, manifest_list_path, file_io_, + format_version >= 2 ? std::optional(sequence_number) : std::nullopt, + format_version >= 3 ? std::optional(kSnapshotFirstRowId) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); EXPECT_THAT(writer->Add(manifest), IsOk()); diff --git a/src/iceberg/test/manifest_list_versions_test.cc b/src/iceberg/test/manifest_list_versions_test.cc index 9c32a59d5..9a741919b 100644 --- a/src/iceberg/test/manifest_list_versions_test.cc +++ b/src/iceberg/test/manifest_list_versions_test.cc @@ -108,21 +108,10 @@ class TestManifestListVersions : public ::testing::Test { const std::string manifest_list_path = CreateManifestListPath(); constexpr int64_t kParentSnapshotId = kSnapshotId - 1; - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 1) { - writer_result = ManifestListWriter::MakeV1Writer(kSnapshotId, kParentSnapshotId, - manifest_list_path, file_io_); - } else if (format_version == 2) { - writer_result = ManifestListWriter::MakeV2Writer( - kSnapshotId, kParentSnapshotId, kSeqNum, manifest_list_path, file_io_); - } else if (format_version == 3) { - writer_result = ManifestListWriter::MakeV3Writer(kSnapshotId, kParentSnapshotId, - kSeqNum, kSnapshotFirstRowId, - manifest_list_path, file_io_); - } - + auto writer_result = ManifestListWriter::MakeWriter( + format_version, kSnapshotId, kParentSnapshotId, manifest_list_path, file_io_, + format_version >= 2 ? std::optional(kSeqNum) : std::nullopt, + format_version >= 3 ? std::optional(kSnapshotFirstRowId) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -202,11 +191,9 @@ class TestManifestListVersions : public ::testing::Test { TEST_F(TestManifestListVersions, TestV1WriteDeleteManifest) { const std::string manifest_list_path = CreateManifestListPath(); - auto writer_result = ManifestListWriter::MakeV1Writer(kSnapshotId, kSnapshotId - 1, - manifest_list_path, file_io_); - EXPECT_THAT(writer_result, IsOk()); - - auto writer = std::move(writer_result.value()); + 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)); diff --git a/src/iceberg/test/manifest_reader_stats_test.cc b/src/iceberg/test/manifest_reader_stats_test.cc index 327da225f..c0ab76151 100644 --- a/src/iceberg/test/manifest_reader_stats_test.cc +++ b/src/iceberg/test/manifest_reader_stats_test.cc @@ -89,26 +89,10 @@ class TestManifestReaderStats : public testing::TestWithParam { ManifestFile WriteManifest(int format_version, std::unique_ptr data_file) { const std::string manifest_path = MakeManifestPath(); - Result> writer_result = - NotSupported("Format version: {}", format_version); - - switch (format_version) { - case 1: - writer_result = ManifestWriter::MakeV1Writer(/*snapshot_id=*/1000L, manifest_path, - file_io_, spec_, schema_); - break; - case 2: - writer_result = - ManifestWriter::MakeV2Writer(/*snapshot_id=*/1000L, manifest_path, file_io_, - spec_, schema_, ManifestContent::kData); - break; - case 3: - writer_result = ManifestWriter::MakeV3Writer( - /*snapshot_id=*/1000L, /*sequence_number=*/0L, manifest_path, file_io_, spec_, - schema_, ManifestContent::kData); - break; - } - + auto writer_result = ManifestWriter::MakeWriter( + format_version, /*snapshot_id=*/1000L, manifest_path, file_io_, spec_, schema_, + ManifestContent::kData, + format_version >= 3 ? std::optional(0L) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); diff --git a/src/iceberg/test/manifest_reader_test.cc b/src/iceberg/test/manifest_reader_test.cc index e8a1a5113..3430ee3aa 100644 --- a/src/iceberg/test/manifest_reader_test.cc +++ b/src/iceberg/test/manifest_reader_test.cc @@ -82,24 +82,10 @@ class TestManifestReader : public testing::TestWithParam { const std::vector& entries) { const std::string manifest_path = MakeManifestPath(); - Result> writer_result = - NotSupported("Format version: {}", format_version); - - switch (format_version) { - case 1: - writer_result = ManifestWriter::MakeV1Writer(snapshot_id, manifest_path, file_io_, - spec_, schema_); - break; - case 2: - writer_result = ManifestWriter::MakeV2Writer( - snapshot_id, manifest_path, file_io_, spec_, schema_, ManifestContent::kData); - break; - case 3: - writer_result = ManifestWriter::MakeV3Writer(snapshot_id, /*first_row_id=*/0L, - manifest_path, file_io_, spec_, - schema_, ManifestContent::kData); - break; - } + auto writer_result = ManifestWriter::MakeWriter( + format_version, snapshot_id, manifest_path, file_io_, spec_, schema_, + ManifestContent::kData, + format_version >= 3 ? std::optional(0L) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -152,18 +138,10 @@ class TestManifestReader : public testing::TestWithParam { std::vector entries) { const std::string manifest_path = MakeManifestPath(); - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 2) { - writer_result = - ManifestWriter::MakeV2Writer(snapshot_id, manifest_path, file_io_, spec_, - schema_, ManifestContent::kDeletes); - } else if (format_version == 3) { - writer_result = ManifestWriter::MakeV3Writer( - snapshot_id, /*first_row_id=*/std::nullopt, manifest_path, file_io_, spec_, - schema_, ManifestContent::kDeletes); - } + auto writer_result = ManifestWriter::MakeWriter( + format_version, snapshot_id, manifest_path, file_io_, spec_, schema_, + ManifestContent::kDeletes, + format_version >= 3 ? std::optional(std::nullopt) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); diff --git a/src/iceberg/test/manifest_writer_versions_test.cc b/src/iceberg/test/manifest_writer_versions_test.cc index e3229fc70..ad68a5033 100644 --- a/src/iceberg/test/manifest_writer_versions_test.cc +++ b/src/iceberg/test/manifest_writer_versions_test.cc @@ -161,21 +161,10 @@ class ManifestWriterVersionsTest : public ::testing::Test { const std::string manifest_list_path = CreateManifestListPath(); constexpr int64_t kParentSnapshotId = kSnapshotId - 1; - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 1) { - writer_result = ManifestListWriter::MakeV1Writer(kSnapshotId, kParentSnapshotId, - manifest_list_path, file_io_); - } else if (format_version == 2) { - writer_result = ManifestListWriter::MakeV2Writer( - kSnapshotId, kParentSnapshotId, kSequenceNumber, manifest_list_path, file_io_); - } else if (format_version == 3) { - writer_result = ManifestListWriter::MakeV3Writer(kSnapshotId, kParentSnapshotId, - kSequenceNumber, kFirstRowId, - manifest_list_path, file_io_); - } - + auto writer_result = ManifestListWriter::MakeWriter( + format_version, kSnapshotId, kParentSnapshotId, manifest_list_path, file_io_, + format_version >= 2 ? std::optional(kSequenceNumber) : std::nullopt, + format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -210,21 +199,10 @@ class ManifestWriterVersionsTest : public ::testing::Test { std::vector> data_files) { const std::string manifest_path = CreateManifestPath(); - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 1) { - writer_result = ManifestWriter::MakeV1Writer(kSnapshotId, manifest_path, file_io_, - spec_, schema_); - } else if (format_version == 2) { - writer_result = ManifestWriter::MakeV2Writer( - kSnapshotId, manifest_path, file_io_, spec_, schema_, ManifestContent::kData); - } else if (format_version == 3) { - writer_result = - ManifestWriter::MakeV3Writer(kSnapshotId, kFirstRowId, manifest_path, file_io_, - spec_, schema_, ManifestContent::kData); - } - + auto writer_result = ManifestWriter::MakeWriter( + format_version, kSnapshotId, manifest_path, file_io_, spec_, schema_, + ManifestContent::kData, + format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -255,18 +233,10 @@ class ManifestWriterVersionsTest : public ::testing::Test { std::shared_ptr delete_file) { const std::string manifest_path = CreateManifestPath(); - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 2) { - writer_result = - ManifestWriter::MakeV2Writer(kSnapshotId, manifest_path, file_io_, spec_, - schema_, ManifestContent::kDeletes); - } else if (format_version == 3) { - writer_result = - ManifestWriter::MakeV3Writer(kSnapshotId, kFirstRowId, manifest_path, file_io_, - spec_, schema_, ManifestContent::kDeletes); - } + auto writer_result = ManifestWriter::MakeWriter( + format_version, kSnapshotId, manifest_path, file_io_, spec_, schema_, + ManifestContent::kDeletes, + format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -286,21 +256,10 @@ class ManifestWriterVersionsTest : public ::testing::Test { const std::string manifest_path = CreateManifestPath(); - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 1) { - writer_result = ManifestWriter::MakeV1Writer(kSnapshotId, manifest_path, file_io_, - spec_, schema_); - } else if (format_version == 2) { - writer_result = ManifestWriter::MakeV2Writer(kSnapshotId, manifest_path, file_io_, - spec_, schema_, old_manifest.content); - } else if (format_version == 3) { - writer_result = - ManifestWriter::MakeV3Writer(kSnapshotId, kFirstRowId, manifest_path, file_io_, - spec_, schema_, old_manifest.content); - } - + auto writer_result = ManifestWriter::MakeWriter( + format_version, kSnapshotId, manifest_path, file_io_, spec_, schema_, + old_manifest.content, + format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -466,11 +425,9 @@ TEST_F(ManifestWriterVersionsTest, TestV1Write) { TEST_F(ManifestWriterVersionsTest, TestV1WriteDelete) { const std::string manifest_path = CreateManifestPath(); - auto writer_result = - ManifestWriter::MakeV1Writer(kSnapshotId, manifest_path, file_io_, spec_, schema_); - - EXPECT_THAT(writer_result, IsOk()); - auto writer = std::move(writer_result.value()); + ICEBERG_UNWRAP_OR_FAIL( + auto writer, ManifestWriter::MakeWriter(1, kSnapshotId, manifest_path, file_io_, + spec_, schema_)); ManifestEntry entry; entry.snapshot_id = kSnapshotId; diff --git a/src/iceberg/test/rolling_manifest_writer_test.cc b/src/iceberg/test/rolling_manifest_writer_test.cc index 8ea13869e..5e39dbedc 100644 --- a/src/iceberg/test/rolling_manifest_writer_test.cc +++ b/src/iceberg/test/rolling_manifest_writer_test.cc @@ -109,22 +109,10 @@ class RollingManifestWriterTest : public ::testing::TestWithParam { int32_t format_version) { return [this, format_version]() -> Result> { const std::string manifest_path = CreateManifestPath(); - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 1) { - writer_result = ManifestWriter::MakeV1Writer(kSnapshotId, manifest_path, file_io_, - spec_, schema_); - } else if (format_version == 2) { - writer_result = ManifestWriter::MakeV2Writer( - kSnapshotId, manifest_path, file_io_, spec_, schema_, ManifestContent::kData); - } else if (format_version == 3) { - writer_result = ManifestWriter::MakeV3Writer(kSnapshotId, kFirstRowId, - manifest_path, file_io_, spec_, - schema_, ManifestContent::kData); - } - - return writer_result; + return ManifestWriter::MakeWriter( + format_version, kSnapshotId, manifest_path, file_io_, spec_, schema_, + ManifestContent::kData, + format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); }; } @@ -132,20 +120,10 @@ class RollingManifestWriterTest : public ::testing::TestWithParam { int32_t format_version) { return [this, format_version]() -> Result> { const std::string manifest_path = CreateManifestPath(); - Result> writer_result = - NotSupported("Format version: {}", format_version); - - if (format_version == 2) { - writer_result = - ManifestWriter::MakeV2Writer(kSnapshotId, manifest_path, file_io_, spec_, - schema_, ManifestContent::kDeletes); - } else if (format_version == 3) { - writer_result = ManifestWriter::MakeV3Writer(kSnapshotId, kFirstRowId, - manifest_path, file_io_, spec_, - schema_, ManifestContent::kDeletes); - } - - return writer_result; + return ManifestWriter::MakeWriter( + format_version, kSnapshotId, manifest_path, file_io_, spec_, schema_, + ManifestContent::kDeletes, + format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); }; } From 1b49fe0d55f3e2d7fa44a4b91cdf7720909e75c6 Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Wed, 7 Jan 2026 14:35:39 +0800 Subject: [PATCH 2/2] fix: remove ternary conditional operator --- src/iceberg/test/delete_file_index_test.cc | 3 +-- src/iceberg/test/manifest_group_test.cc | 20 +++++++--------- .../test/manifest_list_versions_test.cc | 7 +++--- .../test/manifest_reader_stats_test.cc | 3 +-- src/iceberg/test/manifest_reader_test.cc | 16 ++++++------- .../test/manifest_writer_versions_test.cc | 24 ++++++++----------- .../test/rolling_manifest_writer_test.cc | 14 +++++------ 7 files changed, 38 insertions(+), 49 deletions(-) diff --git a/src/iceberg/test/delete_file_index_test.cc b/src/iceberg/test/delete_file_index_test.cc index 10895e258..d5866e27c 100644 --- a/src/iceberg/test/delete_file_index_test.cc +++ b/src/iceberg/test/delete_file_index_test.cc @@ -167,8 +167,7 @@ class DeleteFileIndexTest : public testing::TestWithParam { auto writer_result = ManifestWriter::MakeWriter( format_version, snapshot_id, manifest_path, file_io_, spec, schema_, - ManifestContent::kDeletes, - format_version >= 3 ? std::optional(std::nullopt) : std::nullopt); + ManifestContent::kDeletes, /*first_row_id=*/std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); diff --git a/src/iceberg/test/manifest_group_test.cc b/src/iceberg/test/manifest_group_test.cc index 8c68de1b1..2a062c9ee 100644 --- a/src/iceberg/test/manifest_group_test.cc +++ b/src/iceberg/test/manifest_group_test.cc @@ -135,10 +135,10 @@ class ManifestGroupTest : public testing::TestWithParam { 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); + auto writer_result = + ManifestWriter::MakeWriter(format_version, snapshot_id, manifest_path, file_io_, + spec, schema_, ManifestContent::kData, + /*first_row_id=*/0L); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -157,11 +157,10 @@ class ManifestGroupTest : public testing::TestWithParam { 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::kDeletes, - /*first_row_id=*/format_version >= 3 ? std::optional(std::nullopt) - : std::nullopt); + auto writer_result = + ManifestWriter::MakeWriter(format_version, snapshot_id, manifest_path, file_io_, + spec, schema_, ManifestContent::kDeletes, + /*first_row_id=*/std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -197,8 +196,7 @@ class ManifestGroupTest : public testing::TestWithParam { auto writer_result = ManifestListWriter::MakeWriter( format_version, snapshot_id, kParentSnapshotId, manifest_list_path, file_io_, - format_version >= 2 ? std::optional(sequence_number) : std::nullopt, - format_version >= 3 ? std::optional(kSnapshotFirstRowId) : std::nullopt); + sequence_number, kSnapshotFirstRowId); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); EXPECT_THAT(writer->Add(manifest), IsOk()); diff --git a/src/iceberg/test/manifest_list_versions_test.cc b/src/iceberg/test/manifest_list_versions_test.cc index 9a741919b..f7049fad6 100644 --- a/src/iceberg/test/manifest_list_versions_test.cc +++ b/src/iceberg/test/manifest_list_versions_test.cc @@ -110,8 +110,7 @@ class TestManifestListVersions : public ::testing::Test { auto writer_result = ManifestListWriter::MakeWriter( format_version, kSnapshotId, kParentSnapshotId, manifest_list_path, file_io_, - format_version >= 2 ? std::optional(kSeqNum) : std::nullopt, - format_version >= 3 ? std::optional(kSnapshotFirstRowId) : std::nullopt); + kSeqNum, kSnapshotFirstRowId); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -192,8 +191,8 @@ 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_)); + ManifestListWriter::MakeV1Writer(kSnapshotId, kSnapshotId - 1, + manifest_list_path, file_io_)); auto status = writer->Add(kDeleteManifest); EXPECT_THAT(status, IsError(ErrorKind::kInvalidManifestList)); diff --git a/src/iceberg/test/manifest_reader_stats_test.cc b/src/iceberg/test/manifest_reader_stats_test.cc index c0ab76151..d1da17953 100644 --- a/src/iceberg/test/manifest_reader_stats_test.cc +++ b/src/iceberg/test/manifest_reader_stats_test.cc @@ -91,8 +91,7 @@ class TestManifestReaderStats : public testing::TestWithParam { auto writer_result = ManifestWriter::MakeWriter( format_version, /*snapshot_id=*/1000L, manifest_path, file_io_, spec_, schema_, - ManifestContent::kData, - format_version >= 3 ? std::optional(0L) : std::nullopt); + ManifestContent::kData, /*first_row_id=*/0L); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); diff --git a/src/iceberg/test/manifest_reader_test.cc b/src/iceberg/test/manifest_reader_test.cc index 3430ee3aa..7604eba9f 100644 --- a/src/iceberg/test/manifest_reader_test.cc +++ b/src/iceberg/test/manifest_reader_test.cc @@ -82,10 +82,10 @@ class TestManifestReader : public testing::TestWithParam { const std::vector& entries) { const std::string manifest_path = MakeManifestPath(); - auto writer_result = ManifestWriter::MakeWriter( - format_version, snapshot_id, manifest_path, file_io_, spec_, schema_, - ManifestContent::kData, - format_version >= 3 ? std::optional(0L) : std::nullopt); + auto writer_result = + ManifestWriter::MakeWriter(format_version, snapshot_id, manifest_path, file_io_, + spec_, schema_, ManifestContent::kData, + /*first_row_id=*/0L); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -138,10 +138,10 @@ class TestManifestReader : public testing::TestWithParam { std::vector entries) { const std::string manifest_path = MakeManifestPath(); - auto writer_result = ManifestWriter::MakeWriter( - format_version, snapshot_id, manifest_path, file_io_, spec_, schema_, - ManifestContent::kDeletes, - format_version >= 3 ? std::optional(std::nullopt) : std::nullopt); + auto writer_result = + ManifestWriter::MakeWriter(format_version, snapshot_id, manifest_path, file_io_, + spec_, schema_, ManifestContent::kDeletes, + /*first_row_id=*/std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); diff --git a/src/iceberg/test/manifest_writer_versions_test.cc b/src/iceberg/test/manifest_writer_versions_test.cc index ad68a5033..70d00504a 100644 --- a/src/iceberg/test/manifest_writer_versions_test.cc +++ b/src/iceberg/test/manifest_writer_versions_test.cc @@ -163,8 +163,7 @@ class ManifestWriterVersionsTest : public ::testing::Test { auto writer_result = ManifestListWriter::MakeWriter( format_version, kSnapshotId, kParentSnapshotId, manifest_list_path, file_io_, - format_version >= 2 ? std::optional(kSequenceNumber) : std::nullopt, - format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); + kSequenceNumber, kFirstRowId); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -199,10 +198,9 @@ class ManifestWriterVersionsTest : public ::testing::Test { std::vector> data_files) { const std::string manifest_path = CreateManifestPath(); - auto writer_result = ManifestWriter::MakeWriter( - format_version, kSnapshotId, manifest_path, file_io_, spec_, schema_, - ManifestContent::kData, - format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); + auto writer_result = + ManifestWriter::MakeWriter(format_version, kSnapshotId, manifest_path, file_io_, + spec_, schema_, ManifestContent::kData, kFirstRowId); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -235,8 +233,7 @@ class ManifestWriterVersionsTest : public ::testing::Test { auto writer_result = ManifestWriter::MakeWriter( format_version, kSnapshotId, manifest_path, file_io_, spec_, schema_, - ManifestContent::kDeletes, - format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); + ManifestContent::kDeletes, kFirstRowId); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -256,10 +253,9 @@ class ManifestWriterVersionsTest : public ::testing::Test { const std::string manifest_path = CreateManifestPath(); - auto writer_result = ManifestWriter::MakeWriter( - format_version, kSnapshotId, manifest_path, file_io_, spec_, schema_, - old_manifest.content, - format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); + auto writer_result = + ManifestWriter::MakeWriter(format_version, kSnapshotId, manifest_path, file_io_, + spec_, schema_, old_manifest.content, kFirstRowId); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -426,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::MakeV1Writer(kSnapshotId, manifest_path, file_io_, spec_, schema_)); ManifestEntry entry; entry.snapshot_id = kSnapshotId; diff --git a/src/iceberg/test/rolling_manifest_writer_test.cc b/src/iceberg/test/rolling_manifest_writer_test.cc index 5e39dbedc..439359bc8 100644 --- a/src/iceberg/test/rolling_manifest_writer_test.cc +++ b/src/iceberg/test/rolling_manifest_writer_test.cc @@ -109,10 +109,9 @@ class RollingManifestWriterTest : public ::testing::TestWithParam { int32_t format_version) { return [this, format_version]() -> Result> { const std::string manifest_path = CreateManifestPath(); - return ManifestWriter::MakeWriter( - format_version, kSnapshotId, manifest_path, file_io_, spec_, schema_, - ManifestContent::kData, - format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); + return ManifestWriter::MakeWriter(format_version, kSnapshotId, manifest_path, + file_io_, spec_, schema_, ManifestContent::kData, + kFirstRowId); }; } @@ -120,10 +119,9 @@ class RollingManifestWriterTest : public ::testing::TestWithParam { int32_t format_version) { return [this, format_version]() -> Result> { const std::string manifest_path = CreateManifestPath(); - return ManifestWriter::MakeWriter( - format_version, kSnapshotId, manifest_path, file_io_, spec_, schema_, - ManifestContent::kDeletes, - format_version >= 3 ? std::optional(kFirstRowId) : std::nullopt); + return ManifestWriter::MakeWriter(format_version, kSnapshotId, manifest_path, + file_io_, spec_, schema_, + ManifestContent::kDeletes, kFirstRowId); }; }