You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "zenfenan (via GitHub)" <gi...@apache.org> on 2026/07/14 22:24:02 UTC
[PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
zenfenan opened a new pull request, #3658:
URL: https://github.com/apache/iceberg-python/pull/3658
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace ${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
Closes #3657
# Rationale for this change
`_open_manifest()` currently deserializes all live manifest entries via `fetch_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.
| Total Entries | Entries Matching Filter | Two-pass Time | Fused Time | Improvement |
|-----------------------|------------------------|----------------|-------------|--------------|
| 116k | 78 (0.07%) | 9.13s | 7.18s | -21.3% |
| 438k | 1 (~0%) | 35.12s | 27.81s | -20.8% |
| 823k | 4,962 (0.6%) | 54.38s | 41.38s | -23.9% |
| 18k | 269 (1.5%) | 1.23s | 0.90s | -27.0% |
| 8k | 185 (2.2%) | 0.57s | 0.40s | -30.2% |
## 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.
<!-- In the case of user-facing changes, please add the changelog label. -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3658:
URL: https://github.com/apache/iceberg-python/pull/3658#issuecomment-5383315111
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
Posted by "abnobdoss (via GitHub)" <gi...@apache.org>.
abnobdoss commented on code in PR #3658:
URL: https://github.com/apache/iceberg-python/pull/3658#discussion_r3607317474
##########
pyiceberg/table/__init__.py:
##########
@@ -2151,11 +2151,7 @@ def _open_manifest(
Returns:
A list of ManifestEntry that matches the provided filters.
"""
- return [
Review 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:
```python
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:
1. `table/__init__.py` - `_open_manifest` (partition + metrics)
2. `table/inspect.py` - `_get_files_from_manifest` (content type)
3. `table/update/snapshot.py` - `_OverwriteFiles._build_...` (strict + inclusive metrics)
4. `table/update/validate.py` - `_deleted_data_files` (snapshot_id + data_filter + partition_set)
5. `table/update/validate.py` - `_added_data_files` (same)
6. `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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
Posted by "zenfenan (via GitHub)" <gi...@apache.org>.
zenfenan commented on PR #3658:
URL: https://github.com/apache/iceberg-python/pull/3658#issuecomment-5484810162
@kevinjqliu Could you please re-open this PR, and review this? Highly appreciated!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
Posted by "zenfenan (via GitHub)" <gi...@apache.org>.
zenfenan commented on code in PR #3658:
URL: https://github.com/apache/iceberg-python/pull/3658#discussion_r3633775601
##########
pyiceberg/table/__init__.py:
##########
@@ -2151,11 +2151,7 @@ def _open_manifest(
Returns:
A list of ManifestEntry that matches the provided filters.
"""
- return [
Review Comment:
Thanks for the review! I agree on both points.
My initial approach was a separate method to avoid any risk of side effects to existing callers but the optional `entry_filter` defaulting to `None` achieves the same safety without duplicating the Avro reader setup. I have updated the PR to include this but kept the other call site migration for a follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
Posted by "zenfenan (via GitHub)" <gi...@apache.org>.
zenfenan commented on PR #3658:
URL: https://github.com/apache/iceberg-python/pull/3658#issuecomment-5060877193
@Fokko @kevinjqliu
Whenever you have a free moment, could you please take a look at this PR? All checks are green and previous feedback is addressed. Really appreciate it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3658:
URL: https://github.com/apache/iceberg-python/pull/3658#issuecomment-5465769334
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #3658:
URL: https://github.com/apache/iceberg-python/pull/3658#issuecomment-5505671877
> One thought: a simpler alternative might be to convert fetch_manifest_entry into a generator instead of adding entry_filter. That way _open_manifest keeps its existing comprehension filter and the memory win comes for free, with no new parameter. It would be a wider change though, so not sure what maintainers would prefer here.
In hindsight this might have been a better approach, but that would change the signature. For example, I can see folks doing a `len(..)` operation, but that's not possible on a generator and would raise an exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
Posted by "zenfenan (via GitHub)" <gi...@apache.org>.
zenfenan commented on PR #3658:
URL: https://github.com/apache/iceberg-python/pull/3658#issuecomment-5484796317
@kevinjqliu Could I get a review please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
Posted by "zenfenan (via GitHub)" <gi...@apache.org>.
zenfenan commented on PR #3658:
URL: https://github.com/apache/iceberg-python/pull/3658#issuecomment-5060806650
> Thank you for addressing the feedback. This looks good to me!
>
> One thought: a simpler alternative might be to convert `fetch_manifest_entry` into a generator instead of adding entry_filter. That way _open_manifest keeps its existing comprehension filter and the memory win comes for free, with no new parameter. It would be a wider change though, so not sure what maintainers would prefer here.
Thanks! Good idea. A generator would give the memory win without a new parameter.
Two things make me lean toward keeping the current approach here, though: it changes the return-type contract (callers doing `len()`/indexing including some existing tests would need `list()` wrapping), and it changes resource semantics since the Avro file handle would stay open for the lifetime of iteration rather than being read and closed up front.
Happy to explore the generator conversion as a separate PR if maintainers think the broader change is worth it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #3658:
URL: https://github.com/apache/iceberg-python/pull/3658
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Fuse partition and metrics filtering into manifest deserialization [iceberg-python]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3658: feat: Fuse partition and metrics filtering into manifest deserialization
URL: https://github.com/apache/iceberg-python/pull/3658
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org