Skip to content

Commit 693c8be

Browse files
zhjwpkuzhjwpku
committed
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
1 parent e5eb6e0 commit 693c8be

9 files changed

+150
-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: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,25 @@ 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 version.
162+
/// \param format_version The format version (1, 2, 3, etc.).
163+
/// \param snapshot_id ID of the snapshot.
164+
/// \param manifest_location Path to the manifest file.
165+
/// \param file_io File IO implementation to use.
166+
/// \param partition_spec Partition spec for the manifest.
167+
/// \param current_schema Schema containing the source fields referenced by partition
168+
/// spec.
169+
/// \param content Content of the manifest (required for format_version >= 2).
170+
/// \param first_row_id First row ID of the snapshot (required for format_version >= 3).
171+
/// \return A Result containing the writer or an error.
172+
static Result<std::unique_ptr<ManifestWriter>> MakeWriter(
173+
int8_t format_version, std::optional<int64_t> snapshot_id,
174+
std::string_view manifest_location, std::shared_ptr<FileIO> file_io,
175+
std::shared_ptr<PartitionSpec> partition_spec,
176+
std::shared_ptr<Schema> current_schema,
177+
std::optional<ManifestContent> content = std::nullopt,
178+
std::optional<int64_t> first_row_id = std::nullopt);
179+
161180
private:
162181
// Private constructor for internal use only, use the static Make*Writer methods
163182
// instead.
@@ -240,6 +259,22 @@ class ICEBERG_EXPORT ManifestListWriter {
240259
int64_t sequence_number, int64_t first_row_id,
241260
std::string_view manifest_list_location, std::shared_ptr<FileIO> file_io);
242261

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

‎src/iceberg/test/delete_file_index_test.cc‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -165,17 +165,10 @@ 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,
171+
format_version >= 3 ? std::optional<int64_t>(std::nullopt) : std::nullopt);
179172

180173
EXPECT_THAT(writer_result, IsOk());
181174
auto writer = std::move(writer_result.value());

‎src/iceberg/test/manifest_group_test.cc‎

Lines changed: 13 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 = ManifestWriter::MakeWriter(
139+
format_version, snapshot_id, manifest_path, file_io_, spec, schema_,
140+
ManifestContent::kData,
141+
/*first_row_id=*/format_version >= 3 ? std::optional<int64_t>(0L) : std::nullopt);
153142
EXPECT_THAT(writer_result, IsOk());
154143
auto writer = std::move(writer_result.value());
155144

@@ -168,18 +157,11 @@ 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 = ManifestWriter::MakeWriter(
161+
format_version, snapshot_id, manifest_path, file_io_, spec, schema_,
162+
ManifestContent::kDeletes,
163+
/*first_row_id=*/format_version >= 3 ? std::optional<int64_t>(std::nullopt)
164+
: std::nullopt);
183165
EXPECT_THAT(writer_result, IsOk());
184166
auto writer = std::move(writer_result.value());
185167

@@ -213,21 +195,10 @@ class ManifestGroupTest : public testing::TestWithParam<int> {
213195
constexpr int64_t kParentSnapshotId = 0L;
214196
constexpr int64_t kSnapshotFirstRowId = 0L;
215197

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-
198+
auto writer_result = ManifestListWriter::MakeWriter(
199+
format_version, snapshot_id, kParentSnapshotId, manifest_list_path, file_io_,
200+
format_version >= 2 ? std::optional<int64_t>(sequence_number) : std::nullopt,
201+
format_version >= 3 ? std::optional<int64_t>(kSnapshotFirstRowId) : std::nullopt);
231202
EXPECT_THAT(writer_result, IsOk());
232203
auto writer = std::move(writer_result.value());
233204
EXPECT_THAT(writer->Add(manifest), IsOk());

‎src/iceberg/test/manifest_list_versions_test.cc‎

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -108,21 +108,10 @@ 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+
format_version >= 2 ? std::optional<int64_t>(kSeqNum) : std::nullopt,
114+
format_version >= 3 ? std::optional<int64_t>(kSnapshotFirstRowId) : std::nullopt);
126115
EXPECT_THAT(writer_result, IsOk());
127116
auto writer = std::move(writer_result.value());
128117

@@ -202,11 +191,9 @@ class TestManifestListVersions : public ::testing::Test {
202191
TEST_F(TestManifestListVersions, TestV1WriteDeleteManifest) {
203192
const std::string manifest_list_path = CreateManifestListPath();
204193

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());
194+
ICEBERG_UNWRAP_OR_FAIL(auto writer,
195+
ManifestListWriter::MakeWriter(1, kSnapshotId, kSnapshotId - 1,
196+
manifest_list_path, file_io_));
210197
auto status = writer->Add(kDeleteManifest);
211198

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

‎src/iceberg/test/manifest_reader_stats_test.cc‎

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -89,26 +89,10 @@ 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,
95+
format_version >= 3 ? std::optional<int64_t>(0L) : std::nullopt);
11296
EXPECT_THAT(writer_result, IsOk());
11397
auto writer = std::move(writer_result.value());
11498

‎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 = ManifestWriter::MakeWriter(
86+
format_version, snapshot_id, manifest_path, file_io_, spec_, schema_,
87+
ManifestContent::kData,
88+
format_version >= 3 ? std::optional<int64_t>(0L) : std::nullopt);
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 = ManifestWriter::MakeWriter(
142+
format_version, snapshot_id, manifest_path, file_io_, spec_, schema_,
143+
ManifestContent::kDeletes,
144+
format_version >= 3 ? std::optional<int64_t>(std::nullopt) : std::nullopt);
167145

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

0 commit comments

Comments
 (0)