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..d5866e27c 100644 --- a/src/iceberg/test/delete_file_index_test.cc +++ b/src/iceberg/test/delete_file_index_test.cc @@ -165,17 +165,9 @@ 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, /*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 c7655342b..2a062c9ee 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=*/0L); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -168,18 +157,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 == 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=*/std::nullopt); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -213,21 +194,9 @@ 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_, + 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 9c32a59d5..f7049fad6 100644 --- a/src/iceberg/test/manifest_list_versions_test.cc +++ b/src/iceberg/test/manifest_list_versions_test.cc @@ -108,21 +108,9 @@ 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_, + kSeqNum, kSnapshotFirstRowId); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -202,11 +190,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::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 327da225f..d1da17953 100644 --- a/src/iceberg/test/manifest_reader_stats_test.cc +++ b/src/iceberg/test/manifest_reader_stats_test.cc @@ -89,26 +89,9 @@ 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, /*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 e8a1a5113..7604eba9f 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, + /*first_row_id=*/0L); 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, + /*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 e3229fc70..70d00504a 100644 --- a/src/iceberg/test/manifest_writer_versions_test.cc +++ b/src/iceberg/test/manifest_writer_versions_test.cc @@ -161,21 +161,9 @@ 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_, + kSequenceNumber, kFirstRowId); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -210,21 +198,9 @@ 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, kFirstRowId); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -255,18 +231,9 @@ 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, kFirstRowId); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -286,21 +253,9 @@ 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, kFirstRowId); EXPECT_THAT(writer_result, IsOk()); auto writer = std::move(writer_result.value()); @@ -466,11 +421,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::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 8ea13869e..439359bc8 100644 --- a/src/iceberg/test/rolling_manifest_writer_test.cc +++ b/src/iceberg/test/rolling_manifest_writer_test.cc @@ -109,22 +109,9 @@ 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, + kFirstRowId); }; } @@ -132,20 +119,9 @@ 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, kFirstRowId); }; }