adriangb opened a new pull request, #24188:
URL: https://github.com/apache/datafusion/pull/24188
## Which issue does this PR close?
- N/A — no existing issue; the motivation is written up below. Happy to file
one
if a maintainer prefers to track it separately.
## Rationale for this change
DataFusion has two parquet caches whose working sets scale linearly with
schema
width, but whose limits are fixed byte budgets: `file_statistics_cache`
(default 20 MiB) and `file_metadata_cache` (default 50 MiB). On a
1024-column × 256-file parquet table, measured locally:
- in-memory footer + page-index metadata is ~1.2 MiB/file → a **307 MiB**
working set against the 50 MiB default
- per-file statistics are ~208 KiB/file → **~52 MiB** against the 20 MiB
default
- so both caches sit at a **~0% hit rate**. Worse, the access pattern (a full
sweep over every file, once per query) is LRU's pathological case, so the
degradation is a cliff and not a gradient: raising the metadata limit to
200 MiB against a 307 MiB working set still delivers **0%** of the benefit.
- with both caches at their defaults a representative query took **507 ms**;
with both caches *disabled*, **483 ms**; with both fully resident, **8.8
ms**.
The point of this PR is the last bullet: at the defaults the caches were pure
overhead, and **a user in that regime today has no way to discover it.**
`DefaultCacheState` keeps a per-entry `hits: HashMap<K, usize>`, but there
are
no aggregate counters anywhere, and the only cache introspection in the
project
is datafusion-cli's `metadata_cache()` table function — which lists the
entries
that are currently resident, and therefore says nothing at all about the ones
that were evicted a microsecond after being inserted. Cache thrashing is
currently invisible and undiagnosable.
This PR adds the counters. It does not change any caching behaviour.
## What changes are included in this PR?
Three small commits:
1. **`CacheStatistics` + `Cache::statistics()`** (`cache/mod.rs`). A plain
struct of aggregate, lifetime counters:
| field | meaning |
| --- | --- |
| `hits` | `get` calls that returned a value |
| `misses` | `get` calls that did not — key absent *or* entry expired |
| `evictions` | entries dropped to stay within the byte budget |
| `bytes_evicted` | total size of those entries |
| `inserts_rejected_too_large` | inserts where the entry alone exceeded
the whole limit |
plus `lookups()` and `hit_rate()` helpers.
`inserts_rejected_too_large` is called out separately on purpose: that
path
already exists in `DefaultCacheState::put`, and a non-zero value means the
cache can *never* hold that entry no matter how much of it is free. That
is a
different problem from ordinary eviction pressure and wants a different
fix
(raise the limit vs. shrink the working set), so folding it into
`evictions`
would lose the most diagnostic signal in the set. It is exactly what
happens
when one wide-schema file's metadata exceeds the entire cache budget.
2. **`DefaultCache` tracks them** (`cache/default_cache.rs`).
3. **`CacheManager` exposes them** (`cache/cache_manager.rs`):
`get_file_metadata_cache_statistics()`,
`get_file_statistic_cache_statistics()` and
`get_list_files_cache_statistics()`, alongside the existing `*_limit`
accessors. Each returns `None` when the cache is disabled or when the
configured implementation is not instrumented.
### On not breaking the `Cache` trait
`Cache` is public API with potential external implementors, so
`statistics()` is **a new trait method with a default implementation** rather
than a required one — existing implementations keep compiling untouched. It
returns `Option<CacheStatistics>` (default `None`) rather than
`CacheStatistics::default()` so that "this implementation is not
instrumented"
stays distinguishable from "this instrumented cache has genuinely done
nothing
yet"; reporting all-zero counters for a cache that never counted anything
would
be actively misleading. A test in `cache/mod.rs` implements `Cache` without
overriding `statistics()`, both to pin the non-breaking guarantee and to
assert
the `None`.
### On the hot path
The counters are fields of `DefaultCacheState`, i.e. they live behind the
`Mutex` that every `get`/`put` already takes, and are incremented inside that
existing critical section. No new synchronisation, no atomics, no second
lock.
## Are these changes tested?
Yes — new unit tests in `cache/default_cache.rs`, one per counter:
- `test_statistics_hits_and_misses` — hits, misses on absent keys, `clear()`
neither resetting the counters nor counting as eviction, and that
`contains_key` is a probe that is deliberately *not* counted
- `test_statistics_expired_entry_counts_as_miss` — a TTL-expired entry is a
miss, not an eviction
- `test_statistics_evictions` — `evictions`/`bytes_evicted` for LRU eviction
and
for eviction caused by lowering the limit, and that `remove`,
`drop_table_entries` and same-key replacement are *not* evictions
- `test_statistics_inserts_rejected_too_large` — the rejection path,
including
the case where the rejected insert also drops a stale entry under that key
plus `test_cache_statistics_reachable_from_cache_manager` in
`cache/cache_manager.rs` and the trait-default tests in `cache/mod.rs`.
`cargo test -p datafusion-execution`, `cargo fmt --all` and
`./ci/scripts/rust_clippy.sh` all pass.
## Are there any user-facing changes?
Additive only, no behaviour change:
- new public `CacheStatistics` struct
- new `Cache::statistics()` **with a default implementation**, so this is
not a
breaking change for external implementors
- three new `CacheManager` accessors
Deliberately left out, to keep this reviewable:
- **`EXPLAIN ANALYZE`.** Surfacing cache hit rate per query is the natural
follow-up and the thing that would actually put this in front of users,
but it
needs a decision about per-query vs. process-lifetime accounting (these
counters are process-lifetime and monotonic) and touches the metrics
plumbing.
Separate PR.
- **datafusion-cli's `metadata_cache()`.** That table function emits one row
per
resident entry; aggregate counters do not fit that shape without either
repeating them on every row or adding a second table function. Both grow
the
diff more than they are worth here.
- **A counter reset.** Would be useful for per-query measurement, but only
once
there is a consumer for it — see the `EXPLAIN ANALYZE` item.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
