fix manifest cache #2951
Open
fix manifest cache #2951
+613
−25
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
geruh
reviewed
Jan 25, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Improves manifest-list caching to prevent quadratic memory growth by deduplicating cached ManifestFile objects by manifest_path, addressing the memory issue described in #2325.
Changes:
- Reworked manifest caching to store individual
ManifestFileinstances keyed bymanifest_path(instead of caching whole manifest-list tuples). - Updated/added tests to validate
ManifestFileidentity reuse across repeated reads and across overlapping manifest lists. - Added benchmark tests to measure cache memory growth and deduplication behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
pyiceberg/manifest.py |
Changes cache strategy to dedupe ManifestFile objects by manifest_path and adds a lock for cache access. |
tests/utils/test_manifest.py |
Updates the existing cache test and adds new unit tests for cross-manifest-list deduplication. |
tests/benchmark/test_memory_benchmark.py |
Adds benchmark tests intended to reproduce/guard the memory-growth behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
geruh
approved these changes
Jan 25, 2026
| # We expect about 5-10 KB per iteration for typical workloads | ||
| # The key improvement is that growth is O(N) not O(N²) | ||
| # Threshold of 15KB/iteration based on observed behavior - O(N²) would show ~50KB+/iteration | ||
| assert growth_per_iteration < 15000, ( |
Contributor
There was a problem hiding this comment.
nit: non blocking can make a constant
| https://github.com/apache/iceberg-python/issues/2325 | ||
| The issue: When caching manifest lists as tuples, overlapping ManifestFile objects | ||
| are duplicated across cache entries, causing O(N²) memory growth instead of O(N). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Fix part of #2325
Context: #2325 (comment)
Cache Manifest File object instead of Manifest List (tuple of Manifest Files).
This PR fix the O(N²) cache inefficiency, into the expected O(N) linear growth pattern.
Are these changes tested?
Yes, with benchmark test (
tests/benchmark/test_memory_benchmark.py)Result running from main branch: https://gist.github.com/kevinjqliu/970f4b51a12aaa0318a2671173430736
Result running from this branch: https://gist.github.com/kevinjqliu/24990d18d2cea2fa468597c16bfa27fd
Benchmark Comparison: main vs kevinjqliu/fix-manifest-cache
test_manifest_cache_memory_growthtest_memory_after_gc_with_cache_clearedtest_manifest_cache_deduplication_efficiencyMemory Growth Benchmark (50 append operations)
Memory at Each Iteration
This fix reduces memory growth by ~67%, bringing per-iteration growth from ~27 KB down to ~9 KB.
The improvement comes from caching individual
ManifestFileobjects by theirmanifest_pathinstead of caching entire manifest list tuples. This deduplicatesManifestFileobjects that appear in multiple manifest lists (common after appends).Are there any user-facing changes?