feat: Fuse partition and metrics filtering into manifest deserialization#3658
feat: Fuse partition and metrics filtering into manifest deserialization#3658zenfenan wants to merge 1 commit into
Conversation
fc4e2f1 to
9492802
Compare
abnobdoss
left a comment
There was a problem hiding this comment.
This looks good. My main concern is whether we can extend fetch_manifest_entry and avoid introducing prune_manifest_entry.
| Returns: | ||
| A list of ManifestEntry that matches the provided filters. | ||
| """ | ||
| return [ |
There was a problem hiding this comment.
This is a great find! Would it be better to incorporate this fused behavior directly into fetch_manifest_entry by allowing it to accept an entry_filter: Callable[[ManifestEntry], bool] | None arg? That way the optimization is generic and available to all callers that currently materialize the full list just to discard entries immediately.
The call site in _open_manifest would become:
return manifest.fetch_manifest_entry(
io,
discard_deleted=True,
entry_filter=lambda e: partition_filter(e.data_file) and metrics_evaluator(e.data_file),
)This avoids duplicating the Avro reader setup and _inherit_from_manifest call ordering between fetch_manifest_entry and prune_manifest_entry, and there are 6 other call sites that filter after materializing that could benefit from the same interface:
table/__init__.py-_open_manifest(partition + metrics)table/inspect.py-_get_files_from_manifest(content type)table/update/snapshot.py-_OverwriteFiles._build_...(strict + inclusive metrics)table/update/validate.py-_deleted_data_files(snapshot_id + data_filter + partition_set)table/update/validate.py-_added_data_files(same)table/update/validate.py-_added_delete_files(same)
Using Callable[[ManifestEntry], bool] rather than Callable[[DataFile], bool] covers all of these since the validate callers filter on entry-level fields like snapshot_id and status, not just DataFile.
Closes #3657
Rationale for this change
_open_manifest()currently deserializes all live manifest entries viafetch_manifest_entry()and then applies partitions & metrics filters as a second pass. This materialises the full list before discarding non-matching entries.This PR adds
ManifestFile#prune_manifest_entry()that fuses filtering into the deserialization loop, avoiding the intermediate list allocation for non-matching entries.Are these changes tested?
Yes, existing test suit passes. No behavioral change. Correctness was validated across all benchmark runs (matched entry counts identical between both paths).
Are there any user-facing changes?
No.