Skip to content

Commit d07b756

Browse files
zhjwpkuzhjwpku
authored
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
1 parent 4ac1fa1 commit d07b756

9 files changed

+142
-215
lines changed

‎src/iceberg/manifest/manifest_writer.cc‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,32 @@ Result<std::unique_ptr<ManifestWriter>> ManifestWriter::MakeV3Writer(
365365
std::move(writer), std::move(adapter), manifest_location, first_row_id));
366366
}
367367

368+
Result<std::unique_ptr<ManifestWriter>> ManifestWriter::MakeWriter(
369+
int8_t format_version, std::optional<int64_t> snapshot_id,
370+
std::string_view manifest_location, std::shared_ptr<FileIO> file_io,
371+
std::shared_ptr<PartitionSpec> partition_spec, std::shared_ptr<Schema> current_schema,
372+
std::optional<ManifestContent> content, std::optional<int64_t> first_row_id) {
373+
switch (format_version) {
374+
case 1:
375+
return MakeV1Writer(snapshot_id, manifest_location, std::move(file_io),
376+
std::move(partition_spec), std::move(current_schema));
377+
case 2:
378+
ICEBERG_PRECHECK(content.has_value(),
379+
"ManifestContent is required for format version 2");
380+
return MakeV2Writer(snapshot_id, manifest_location, std::move(file_io),
381+
std::move(partition_spec), std::move(current_schema),
382+
content.value());
383+
case 3:
384+
ICEBERG_PRECHECK(content.has_value(),
385+
"ManifestContent is required for format version 3");
386+
return MakeV3Writer(snapshot_id, first_row_id, manifest_location,
387+
std::move(file_io), std::move(partition_spec),
388+
std::move(current_schema), content.value());
389+
default:
390+
return NotSupported("Format version {} is not supported", format_version);
391+
}
392+
}
393+
368394
ManifestListWriter::ManifestListWriter(std::unique_ptr<Writer> writer,
369395
std::unique_ptr<ManifestFileAdapter> adapter)
370396
: writer_(std::move(writer)), adapter_(std::move(adapter)) {}
@@ -452,4 +478,30 @@ Result<std::unique_ptr<ManifestListWriter>> ManifestListWriter::MakeV3Writer(
452478
new ManifestListWriter(std::move(writer), std::move(adapter)));
453479
}
454480

481+
Result<std::unique_ptr<ManifestListWriter>> ManifestListWriter::MakeWriter(
482+
int8_t format_version, int64_t snapshot_id, std::optional<int64_t> parent_snapshot_id,
483+
std::string_view manifest_list_location, std::shared_ptr<FileIO> file_io,
484+
std::optional<int64_t> sequence_number, std::optional<int64_t> first_row_id) {
485+
switch (format_version) {
486+
case 1:
487+
return MakeV1Writer(snapshot_id, parent_snapshot_id, manifest_list_location,
488+
std::move(file_io));
489+
case 2:
490+
ICEBERG_PRECHECK(sequence_number.has_value(),
491+
"Sequence number is required for format version 2");
492+
return MakeV2Writer(snapshot_id, parent_snapshot_id, sequence_number.value(),
493+
manifest_list_location, std::move(file_io));
494+
case 3:
495+
ICEBERG_PRECHECK(sequence_number.has_value(),
496+
"Sequence number is required for format version 3");
497+
ICEBERG_PRECHECK(first_row_id.has_value(),
498+
"First row ID is required for format version 3");
499+
return MakeV3Writer(snapshot_id, parent_snapshot_id, sequence_number.value(),
500+
first_row_id.value(), manifest_list_location,
501+
std::move(file_io));
502+
default:
503+
return NotSupported("Format version {} is not supported", format_version);
504+
}
505+
}
506+
455507
} // namespace iceberg

‎src/iceberg/manifest/manifest_writer.h‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,26 @@ class ICEBERG_EXPORT ManifestWriter {
158158
std::shared_ptr<PartitionSpec> partition_spec,
159159
std::shared_ptr<Schema> current_schema, ManifestContent content);
160160

161+
/// \brief Factory function to create a writer for a manifest file based on format
162+
/// version.
163+
/// \param format_version The format version (1, 2, 3, etc.).
164+
/// \param snapshot_id ID of the snapshot.
165+
/// \param manifest_location Path to the manifest file.
166+
/// \param file_io File IO implementation to use.
167+
/// \param partition_spec Partition spec for the manifest.
168+
/// \param current_schema Schema containing the source fields referenced by partition
169+
/// spec.
170+
/// \param content Content of the manifest (required for format_version >= 2).
171+
/// \param first_row_id First row ID of the snapshot (required for format_version >= 3).
172+
/// \return A Result containing the writer or an error.
173+
static Result<std::unique_ptr<ManifestWriter>> MakeWriter(
174+
int8_t format_version, std::optional<int64_t> snapshot_id,
175+
std::string_view manifest_location, std::shared_ptr<FileIO> file_io,
176+
std::shared_ptr<PartitionSpec> partition_spec,
177+
std::shared_ptr<Schema> current_schema,
178+
std::optional<ManifestContent> content = std::nullopt,
179+
std::optional<int64_t> first_row_id = std::nullopt);
180+
161181
private:
162182
// Private constructor for internal use only, use the static Make*Writer methods
163183
// instead.
@@ -240,6 +260,24 @@ class ICEBERG_EXPORT ManifestListWriter {
240260
int64_t sequence_number, int64_t first_row_id,
241261
std::string_view manifest_list_location, std::shared_ptr<FileIO> file_io);
242262

263+
/// \brief Factory function to create a writer for the manifest list based on format
264+
/// version.
265+
/// \param format_version The format version (1, 2, 3, etc.).
266+
/// \param snapshot_id ID of the snapshot.
267+
/// \param parent_snapshot_id ID of the parent snapshot.
268+
/// \param manifest_list_location Path to the manifest list file.
269+
/// \param file_io File IO implementation to use.
270+
/// \param sequence_number Sequence number of the snapshot (required for format_version
271+
/// >= 2).
272+
/// \param first_row_id First row ID of the snapshot (required for format_version >= 3).
273+
/// \return A Result containing the writer or an error.
274+
static Result<std::unique_ptr<ManifestListWriter>> MakeWriter(
275+
int8_t format_version, int64_t snapshot_id,
276+
std::optional<int64_t> parent_snapshot_id, std::string_view manifest_list_location,
277+
std::shared_ptr<FileIO> file_io,
278+
std::optional<int64_t> sequence_number = std::nullopt,
279+
std::optional<int64_t> first_row_id = std::nullopt);
280+
243281
private:
244282
// Private constructor for internal use only, use the static Make*Writer methods
245283
// instead.

‎src/iceberg/test/delete_file_index_test.cc‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -165,17 +165,9 @@ class DeleteFileIndexTest : public testing::TestWithParam<int> {
165165
std::shared_ptr<PartitionSpec> spec) {
166166
const std::string manifest_path = MakeManifestPath();
167167

168-
Result<std::unique_ptr<ManifestWriter>> writer_result =
169-
NotSupported("Format version: {}", format_version);
170-
171-
if (format_version == 2) {
172-
writer_result = ManifestWriter::MakeV2Writer(
173-
snapshot_id, manifest_path, file_io_, spec, schema_, ManifestContent::kDeletes);
174-
} else if (format_version == 3) {
175-
writer_result = ManifestWriter::MakeV3Writer(
176-
snapshot_id, /*first_row_id=*/std::nullopt, manifest_path, file_io_, spec,
177-
schema_, ManifestContent::kDeletes);
178-
}
168+
auto writer_result = ManifestWriter::MakeWriter(
169+
format_version, snapshot_id, manifest_path, file_io_, spec, schema_,
170+
ManifestContent::kDeletes, /*first_row_id=*/std::nullopt);
179171

180172
EXPECT_THAT(writer_result, IsOk());
181173
auto writer = std::move(writer_result.value());

‎src/iceberg/test/manifest_group_test.cc‎

Lines changed: 11 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -135,21 +135,10 @@ class ManifestGroupTest : public testing::TestWithParam<int> {
135135
std::shared_ptr<PartitionSpec> spec) {
136136
const std::string manifest_path = MakeManifestPath();
137137

138-
Result<std::unique_ptr<ManifestWriter>> writer_result =
139-
NotSupported("Format version: {}", format_version);
140-
141-
if (format_version == 1) {
142-
writer_result = ManifestWriter::MakeV1Writer(snapshot_id, manifest_path, file_io_,
143-
spec, schema_);
144-
} else if (format_version == 2) {
145-
writer_result = ManifestWriter::MakeV2Writer(snapshot_id, manifest_path, file_io_,
146-
spec, schema_, ManifestContent::kData);
147-
} else if (format_version == 3) {
148-
writer_result =
149-
ManifestWriter::MakeV3Writer(snapshot_id, /*first_row_id=*/0L, manifest_path,
150-
file_io_, spec, schema_, ManifestContent::kData);
151-
}
152-
138+
auto writer_result =
139+
ManifestWriter::MakeWriter(format_version, snapshot_id, manifest_path, file_io_,
140+
spec, schema_, ManifestContent::kData,
141+
/*first_row_id=*/0L);
153142
EXPECT_THAT(writer_result, IsOk());
154143
auto writer = std::move(writer_result.value());
155144

@@ -168,18 +157,10 @@ class ManifestGroupTest : public testing::TestWithParam<int> {
168157
std::shared_ptr<PartitionSpec> spec) {
169158
const std::string manifest_path = MakeManifestPath();
170159

171-
Result<std::unique_ptr<ManifestWriter>> writer_result =
172-
NotSupported("Format version: {}", format_version);
173-
174-
if (format_version == 2) {
175-
writer_result = ManifestWriter::MakeV2Writer(
176-
snapshot_id, manifest_path, file_io_, spec, schema_, ManifestContent::kDeletes);
177-
} else if (format_version == 3) {
178-
writer_result = ManifestWriter::MakeV3Writer(
179-
snapshot_id, /*first_row_id=*/std::nullopt, manifest_path, file_io_, spec,
180-
schema_, ManifestContent::kDeletes);
181-
}
182-
160+
auto writer_result =
161+
ManifestWriter::MakeWriter(format_version, snapshot_id, manifest_path, file_io_,
162+
spec, schema_, ManifestContent::kDeletes,
163+
/*first_row_id=*/std::nullopt);
183164
EXPECT_THAT(writer_result, IsOk());
184165
auto writer = std::move(writer_result.value());
185166

@@ -213,21 +194,9 @@ class ManifestGroupTest : public testing::TestWithParam<int> {
213194
constexpr int64_t kParentSnapshotId = 0L;
214195
constexpr int64_t kSnapshotFirstRowId = 0L;
215196

216-
Result<std::unique_ptr<ManifestListWriter>> writer_result =
217-
NotSupported("Format version: {}", format_version);
218-
219-
if (format_version == 1) {
220-
writer_result = ManifestListWriter::MakeV1Writer(snapshot_id, kParentSnapshotId,
221-
manifest_list_path, file_io_);
222-
} else if (format_version == 2) {
223-
writer_result = ManifestListWriter::MakeV2Writer(
224-
snapshot_id, kParentSnapshotId, sequence_number, manifest_list_path, file_io_);
225-
} else if (format_version == 3) {
226-
writer_result = ManifestListWriter::MakeV3Writer(
227-
snapshot_id, kParentSnapshotId, sequence_number, kSnapshotFirstRowId,
228-
manifest_list_path, file_io_);
229-
}
230-
197+
auto writer_result = ManifestListWriter::MakeWriter(
198+
format_version, snapshot_id, kParentSnapshotId, manifest_list_path, file_io_,
199+
sequence_number, kSnapshotFirstRowId);
231200
EXPECT_THAT(writer_result, IsOk());
232201
auto writer = std::move(writer_result.value());
233202
EXPECT_THAT(writer->Add(manifest), IsOk());

‎src/iceberg/test/manifest_list_versions_test.cc‎

Lines changed: 6 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -108,21 +108,9 @@ class TestManifestListVersions : public ::testing::Test {
108108
const std::string manifest_list_path = CreateManifestListPath();
109109
constexpr int64_t kParentSnapshotId = kSnapshotId - 1;
110110

111-
Result<std::unique_ptr<ManifestListWriter>> writer_result =
112-
NotSupported("Format version: {}", format_version);
113-
114-
if (format_version == 1) {
115-
writer_result = ManifestListWriter::MakeV1Writer(kSnapshotId, kParentSnapshotId,
116-
manifest_list_path, file_io_);
117-
} else if (format_version == 2) {
118-
writer_result = ManifestListWriter::MakeV2Writer(
119-
kSnapshotId, kParentSnapshotId, kSeqNum, manifest_list_path, file_io_);
120-
} else if (format_version == 3) {
121-
writer_result = ManifestListWriter::MakeV3Writer(kSnapshotId, kParentSnapshotId,
122-
kSeqNum, kSnapshotFirstRowId,
123-
manifest_list_path, file_io_);
124-
}
125-
111+
auto writer_result = ManifestListWriter::MakeWriter(
112+
format_version, kSnapshotId, kParentSnapshotId, manifest_list_path, file_io_,
113+
kSeqNum, kSnapshotFirstRowId);
126114
EXPECT_THAT(writer_result, IsOk());
127115
auto writer = std::move(writer_result.value());
128116

@@ -202,11 +190,9 @@ class TestManifestListVersions : public ::testing::Test {
202190
TEST_F(TestManifestListVersions, TestV1WriteDeleteManifest) {
203191
const std::string manifest_list_path = CreateManifestListPath();
204192

205-
auto writer_result = ManifestListWriter::MakeV1Writer(kSnapshotId, kSnapshotId - 1,
206-
manifest_list_path, file_io_);
207-
EXPECT_THAT(writer_result, IsOk());
208-
209-
auto writer = std::move(writer_result.value());
193+
ICEBERG_UNWRAP_OR_FAIL(auto writer,
194+
ManifestListWriter::MakeV1Writer(kSnapshotId, kSnapshotId - 1,
195+
manifest_list_path, file_io_));
210196
auto status = writer->Add(kDeleteManifest);
211197

212198
EXPECT_THAT(status, IsError(ErrorKind::kInvalidManifestList));

‎src/iceberg/test/manifest_reader_stats_test.cc‎

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -89,26 +89,9 @@ class TestManifestReaderStats : public testing::TestWithParam<int> {
8989
ManifestFile WriteManifest(int format_version, std::unique_ptr<DataFile> data_file) {
9090
const std::string manifest_path = MakeManifestPath();
9191

92-
Result<std::unique_ptr<ManifestWriter>> writer_result =
93-
NotSupported("Format version: {}", format_version);
94-
95-
switch (format_version) {
96-
case 1:
97-
writer_result = ManifestWriter::MakeV1Writer(/*snapshot_id=*/1000L, manifest_path,
98-
file_io_, spec_, schema_);
99-
break;
100-
case 2:
101-
writer_result =
102-
ManifestWriter::MakeV2Writer(/*snapshot_id=*/1000L, manifest_path, file_io_,
103-
spec_, schema_, ManifestContent::kData);
104-
break;
105-
case 3:
106-
writer_result = ManifestWriter::MakeV3Writer(
107-
/*snapshot_id=*/1000L, /*sequence_number=*/0L, manifest_path, file_io_, spec_,
108-
schema_, ManifestContent::kData);
109-
break;
110-
}
111-
92+
auto writer_result = ManifestWriter::MakeWriter(
93+
format_version, /*snapshot_id=*/1000L, manifest_path, file_io_, spec_, schema_,
94+
ManifestContent::kData, /*first_row_id=*/0L);
11295
EXPECT_THAT(writer_result, IsOk());
11396
auto writer = std::move(writer_result.value());
11497

‎src/iceberg/test/manifest_reader_test.cc‎

Lines changed: 8 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -82,24 +82,10 @@ class TestManifestReader : public testing::TestWithParam<int> {
8282
const std::vector<ManifestEntry>& entries) {
8383
const std::string manifest_path = MakeManifestPath();
8484

85-
Result<std::unique_ptr<ManifestWriter>> writer_result =
86-
NotSupported("Format version: {}", format_version);
87-
88-
switch (format_version) {
89-
case 1:
90-
writer_result = ManifestWriter::MakeV1Writer(snapshot_id, manifest_path, file_io_,
91-
spec_, schema_);
92-
break;
93-
case 2:
94-
writer_result = ManifestWriter::MakeV2Writer(
95-
snapshot_id, manifest_path, file_io_, spec_, schema_, ManifestContent::kData);
96-
break;
97-
case 3:
98-
writer_result = ManifestWriter::MakeV3Writer(snapshot_id, /*first_row_id=*/0L,
99-
manifest_path, file_io_, spec_,
100-
schema_, ManifestContent::kData);
101-
break;
102-
}
85+
auto writer_result =
86+
ManifestWriter::MakeWriter(format_version, snapshot_id, manifest_path, file_io_,
87+
spec_, schema_, ManifestContent::kData,
88+
/*first_row_id=*/0L);
10389

10490
EXPECT_THAT(writer_result, IsOk());
10591
auto writer = std::move(writer_result.value());
@@ -152,18 +138,10 @@ class TestManifestReader : public testing::TestWithParam<int> {
152138
std::vector<ManifestEntry> entries) {
153139
const std::string manifest_path = MakeManifestPath();
154140

155-
Result<std::unique_ptr<ManifestWriter>> writer_result =
156-
NotSupported("Format version: {}", format_version);
157-
158-
if (format_version == 2) {
159-
writer_result =
160-
ManifestWriter::MakeV2Writer(snapshot_id, manifest_path, file_io_, spec_,
161-
schema_, ManifestContent::kDeletes);
162-
} else if (format_version == 3) {
163-
writer_result = ManifestWriter::MakeV3Writer(
164-
snapshot_id, /*first_row_id=*/std::nullopt, manifest_path, file_io_, spec_,
165-
schema_, ManifestContent::kDeletes);
166-
}
141+
auto writer_result =
142+
ManifestWriter::MakeWriter(format_version, snapshot_id, manifest_path, file_io_,
143+
spec_, schema_, ManifestContent::kDeletes,
144+
/*first_row_id=*/std::nullopt);
167145

168146
EXPECT_THAT(writer_result, IsOk());
169147
auto writer = std::move(writer_result.value());

0 commit comments

Comments
 (0)