Skip to content

Conversation

@zhjwpku
Copy link
Collaborator

@zhjwpku zhjwpku commented Jan 7, 2026

  • 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

…iter

- 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
@zhjwpku zhjwpku force-pushed the introduce_writer_factory_function branch from 693c8be to 73e71b8 Compare January 7, 2026 05:29
@zhjwpku zhjwpku changed the title Add factory functions for ManifestWriter and ManifestListWriter feat: Add factory functions for ManifestWriter and ManifestListWriter Jan 7, 2026
@zhjwpku zhjwpku force-pushed the introduce_writer_factory_function branch from 21c9f11 to 1b49fe0 Compare January 7, 2026 06:37
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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that ManifestContent content = ManifestContent::kData is a better option?

@wgtmac wgtmac merged commit d07b756 into apache:main Jan 8, 2026
10 checks passed
@zhjwpku zhjwpku deleted the introduce_writer_factory_function branch January 9, 2026 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants