Uh oh!
There was an error while loading. Please reload this page.
Add a memory bound FileStatisticsCache for the Listing Table - #20047
Conversation
e273afc to
b297378CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
mkleen
commented
Feb 4, 2026
@kosiew Thank you for the feedback! |
mkleen
commented
Feb 10, 2026
@kosiew Anything else needed to get this merged? Another approval maybe? |
205f96c to
92899a7Compare| impl<T: DFHeapSize> DFHeapSize for Arc<T> { | ||
| fn heap_size(&self) -> usize { | ||
| // Arc stores weak and strong counts on the heap alongside an instance of T | ||
| 2 * size_of::<usize>() + size_of::<T>() + self.as_ref().heap_size() |
There was a problem hiding this comment.
This won't be accurate.
let a1 = Arc::new(vec![1,2,3]);let a2 = a1.clone();let a3 = a1.clone();let a4 = a3.clone();// this should be true because all `a`s point to the same object in memory // but the current implementation does not detect this and counts them separatelyassert_eq!(a4.heap_size(), a1.heap_size() + a2.heap_size() + a3.heap_size() + a4.heap_size());The only solution I imagine is the caller to keep track of the pointer addresses which have been "sized" and ignore any Arc's which point to an address which has been "sized" earlier.
There was a problem hiding this comment.
Good catch! I took this implementation from https://github.com/apache/arrow-rs/blob/main/parquet/src/file/metadata/memory.rs#L97-L102 . I would suggest to also do a follow-up here. We are planing anyway to restructure the whole heap size estimation.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
mkleen
commented
Feb 10, 2026
@martin-g Thanks for this great review! I am on it. |
92899a7 to
2e3aff9Comparemkleen
commented
May 15, 2026
Hi @kosiew This is ready for another review. I fixed all remaining open issues. |
kosiew
commented
May 15, 2026
run benchmark clickbench_partitioned |
adriangbot
commented
May 15, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing file-stats-cache (f648bee) to 9d92944 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
mkleen
commented
May 15, 2026
@kosiew Thank you! |
adriangbot
commented
May 15, 2026
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
kosiew
commented
May 15, 2026
@mkleen |
mkleen
commented
May 15, 2026
@kosiew Thank you for all the work and the reviews! |
alamb
commented
May 15, 2026
run benchmark clickbench_partitioned |
alamb
commented
May 15, 2026
I want to see if some of these slowdowns are reproduced |
adriangbot
commented
May 15, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing file-stats-cache (f648bee) to 9d92944 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
adriangbot
commented
May 15, 2026
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
@alamb It looks to me that the slowdowns are not real, but only noise. |
alamb
commented
May 18, 2026
Agreed |
alamb
commented
May 18, 2026
@nuno-faria and @kosiew -- is there anything else we need to do before merging this PR? |
nuno-faria
commented
May 18, 2026
Not on my end. Thanks again @mkleen for the hard work! |
alamb
commented
May 18, 2026
All right, adding to the merge queue to get this in for 54. Thanks again @mkleen |
alamb
commented
May 20, 2026
🎉 -- amazing - Thank you so much @mkleen and @nuno-faria and @kosiew |
## Which issue does this PR close? None. ## Rationale for this change This is a follow-up for apache#20047 improving docs and test coverage for the heap size estimation. ## What changes are included in this PR? See above. ## Are these changes tested? Yes. ## Are there any user-facing changes? No.
…2950, adapted) (#23573) ## Which issue does this PR close? - Backport of #22950 to `branch-54` (for 54.1.0, tracked in #22547). - Fixes panic #22935 (reporter bisected to a regression between 53.1.0 and 54.0.0; introduced by #20047). ## Rationale for this change Anonymous file reads can read the same path with different explicit schemas in one session. The shared file-statistics cache did not validate that cached statistics matched the read schema, so a narrower cached entry could be reused for a later wider-schema read and panic during statistics projection (`index out of bounds` in projection). No API change, so it fits the backport criteria. ## What changes are included in this PR? This is an **adapted** backport. The core fix (`table.rs`, `options.rs`, `context/mod.rs`) is applied; two conflicts were resolved for `branch-54`'s API shape: - `ListingTable::statistics_cache()` returns `Option<&Arc<dyn FileStatisticsCache>>` (on `branch-54`, `FileStatisticsCache` is a trait, not a concrete type as on `main`). - Kept `branch-54`'s import paths for `TableScopedPath` / `FileStatisticsCache`. - The regression test `anonymous_parquet_stats_cache_with_explicit_wider_schema` computes statistics via `plan.partition_statistics(None)` instead of `statistics_with_args`, which does not exist on `branch-54`. ## Are these changes tested? Yes. The regression test passes with the fix and, without it, reproduces the exact reported panic (`index out of bounds: the len is 2 but the index is 2`), confirming it exercises the regression. ## Are there any user-facing changes? No API change. Fixes a panic on anonymous multi-schema reads of the same path. Co-authored-by: Kumar Ujjawal <ujjawalpathak6@gmail.com>
Which issue does this PR close?
This change introduces a default
FileStatisticsCacheimplementation for the Listing-Table with a size limit, implementing the following steps following #19052 (comment) :Add heap size estimation for file statistics and the relevant data types used in caching (This is temporary until Add a crate for HeapSize trait arrow-rs#9138 is resolved)
Redesign
DefaultFileStatisticsCacheto use aLruQueueto make it memory-bound following Adds memory-bound DefaultListFilesCache #18855Introduce a size limit and use it together with the heap-size to limit the memory usage of the cache
Move
FileStatisticsCachecreation intoCacheManager, making it session-scoped and shared across statements and tablesClosesAdd a default
FileStatisticsCacheimplementation for theListingTable#19217ClosesAdd limit to
DefaultFileStatisticsCache#19052Rationale for this change
See above.
What changes are included in this PR?
See above.
Are these changes tested?
Yes.
Are there any user-facing changes?
A new runtime setting
datafusion.runtime.file_statistics.cache_limit