This is an automated email from the ASF dual-hosted git repository.
gangwu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new d07b7564 feat: Add factory functions for ManifestWriter and
ManifestListWriter (#493)
d07b7564 is described below
commit d07b7564ab170078e308c59f7ba3401be86f273f
Author: Junwang Zhao <[email protected]>
AuthorDate: Thu Jan 8 21:56:00 2026 +0800
feat: Add factory functions for ManifestWriter and ManifestListWriter (#493)
- 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 | 14 +----
src/iceberg/test/manifest_group_test.cc | 53 ++++------------
src/iceberg/test/manifest_list_versions_test.cc | 26 ++------
src/iceberg/test/manifest_reader_stats_test.cc | 23 +------
src/iceberg/test/manifest_reader_test.cc | 38 +++--------
src/iceberg/test/manifest_writer_versions_test.cc | 77 +++++------------------
src/iceberg/test/rolling_manifest_writer_test.cc | 36 ++---------
9 files changed, 142 insertions(+), 215 deletions(-)
diff --git a/src/iceberg/manifest/manifest_writer.cc
b/src/iceberg/manifest/manifest_writer.cc
index 649797fe..0899869a 100644
--- a/src/iceberg/manifest/manifest_writer.cc
+++ b/src/iceberg/manifest/manifest_writer.cc
@@ -365,6 +365,32 @@ Result<std::unique_ptr<ManifestWriter>>
ManifestWriter::MakeV3Writer(
std::move(writer), std::move(adapter), manifest_location, first_row_id));
}
+Result<std::unique_ptr<ManifestWriter>> ManifestWriter::MakeWriter(
+ int8_t format_version, std::optional<int64_t> snapshot_id,
+ std::string_view manifest_location, std::shared_ptr<FileIO> file_io,
+ std::shared_ptr<PartitionSpec> partition_spec, std::shared_ptr<Schema>
current_schema,
+ std::optional<ManifestContent> content, std::optional<int64_t>
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> writer,
std::unique_ptr<ManifestFileAdapter>
adapter)
: writer_(std::move(writer)), adapter_(std::move(adapter)) {}
@@ -452,4 +478,30 @@ Result<std::unique_ptr<ManifestListWriter>>
ManifestListWriter::MakeV3Writer(
new ManifestListWriter(std::move(writer), std::move(adapter)));
}
+Result<std::unique_ptr<ManifestListWriter>> ManifestListWriter::MakeWriter(
+ int8_t format_version, int64_t snapshot_id, std::optional<int64_t>
parent_snapshot_id,
+ std::string_view manifest_list_location, std::shared_ptr<FileIO> file_io,
+ std::optional<int64_t> sequence_number, std::optional<int64_t>
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 6f468240..5a095b28 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<PartitionSpec> partition_spec,
std::shared_ptr<Schema> 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<std::unique_ptr<ManifestWriter>> MakeWriter(
+ int8_t format_version, std::optional<int64_t> snapshot_id,
+ std::string_view manifest_location, std::shared_ptr<FileIO> file_io,
+ std::shared_ptr<PartitionSpec> partition_spec,
+ std::shared_ptr<Schema> current_schema,
+ std::optional<ManifestContent> content = std::nullopt,
+ std::optional<int64_t> 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<FileIO>
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<std::unique_ptr<ManifestListWriter>> MakeWriter(
+ int8_t format_version, int64_t snapshot_id,
+ std::optional<int64_t> parent_snapshot_id, std::string_view
manifest_list_location,
+ std::shared_ptr<FileIO> file_io,
+ std::optional<int64_t> sequence_number = std::nullopt,
+ std::optional<int64_t> 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 cf75fe2d..d5866e27 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<int> {
std::shared_ptr<PartitionSpec> spec) {
const std::string manifest_path = MakeManifestPath();
- Result<std::unique_ptr<ManifestWriter>> 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 c7655342..2a062c9e 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<int> {
std::shared_ptr<PartitionSpec> spec) {
const std::string manifest_path = MakeManifestPath();
- Result<std::unique_ptr<ManifestWriter>> 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<int> {
std::shared_ptr<PartitionSpec> spec) {
const std::string manifest_path = MakeManifestPath();
- Result<std::unique_ptr<ManifestWriter>> 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<int> {
constexpr int64_t kParentSnapshotId = 0L;
constexpr int64_t kSnapshotFirstRowId = 0L;
- Result<std::unique_ptr<ManifestListWriter>> 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 9c32a59d..f7049fad 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<std::unique_ptr<ManifestListWriter>> 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 327da225..d1da1795 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<int> {
ManifestFile WriteManifest(int format_version, std::unique_ptr<DataFile>
data_file) {
const std::string manifest_path = MakeManifestPath();
- Result<std::unique_ptr<ManifestWriter>> 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 e8a1a511..7604eba9 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<int> {
const std::vector<ManifestEntry>& entries) {
const std::string manifest_path = MakeManifestPath();
- Result<std::unique_ptr<ManifestWriter>> 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<int> {
std::vector<ManifestEntry> entries) {
const std::string manifest_path = MakeManifestPath();
- Result<std::unique_ptr<ManifestWriter>> 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 e3229fc7..70d00504 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<std::unique_ptr<ManifestListWriter>> 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<std::shared_ptr<DataFile>>
data_files) {
const std::string manifest_path = CreateManifestPath();
- Result<std::unique_ptr<ManifestWriter>> 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<DataFile> delete_file) {
const std::string manifest_path = CreateManifestPath();
- Result<std::unique_ptr<ManifestWriter>> 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<std::unique_ptr<ManifestWriter>> 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 8ea13869..439359bc 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> {
int32_t format_version) {
return [this, format_version]() -> Result<std::unique_ptr<ManifestWriter>>
{
const std::string manifest_path = CreateManifestPath();
- Result<std::unique_ptr<ManifestWriter>> 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> {
int32_t format_version) {
return [this, format_version]() -> Result<std::unique_ptr<ManifestWriter>>
{
const std::string manifest_path = CreateManifestPath();
- Result<std::unique_ptr<ManifestWriter>> 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);
};
}
