Repository navigation
Introduce generic memory-limiting cache for parquet metadata - #22613
Merged
Merged
Conversation
mkleen
force-pushed
the
generic-cache-1
branch
2 times, most recently
from
May 29, 2026 08:07
f0af85c to
8283e19
Compare
mkleen
commented
May 29, 2026
mkleen
commented
May 29, 2026
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
mkleen
commented
May 29, 2026
| DEFAULT_LIST_FILES_CACHE_MEMORY_LIMIT, DEFAULT_LIST_FILES_CACHE_TTL, | ||
| }; | ||
| /// Calculates the number of bytes an [`ObjectMeta`] occupies in the heap. | ||
| pub fn meta_heap_bytes(object_meta: &ObjectMeta) -> usize { |
Contributor
Author
There was a problem hiding this comment.
This is a left-over from list-files-cache which didn't use the DFHeapsize trait for heap-size accounting because it did not exist at the time. DFHeapsize cannot be implemented here for ObjectMeta because it would become an orphan. To remove it and use DFHeapsize this would require one of the following changes:
-
Add a dependency to
object_storeto the packagecommonswhereDFHeapsizeis located and implement it forObjectMeta. The downside is that this would spreadobject_storearound other depending packages. -
Move the
DFHeapsizetrait into theexecutionpackage where the cache is located, since it's nowhere else used yet. -
Create a new crate/package for the heap-size estimation and add the
object_storedependency there and use it from there.
Contributor
There was a problem hiding this comment.
I'm also not a fan of this random function here. Could we at least move it near impl Value for CachedFileList?
Contributor
Author
There was a problem hiding this comment.
@alamb Do you have any strong opinion here? Thanks!
mkleen
force-pushed
the
generic-cache-1
branch
3 times, most recently
from
May 29, 2026 09:23
29d2f82 to
5920922
Compare
mkleen
marked this pull request as ready for review
May 29, 2026 09:53
Contributor
Author
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing generic-cache-1 (5920922) to 32a1fe5 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing generic-cache-1 (5920922) to 32a1fe5 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing generic-cache-1 (5920922) to 32a1fe5 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmark clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing generic-cache-1 (5920922) to 32a1fe5 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
Author
|
@nuno-faria Do you have maybe time for a review for this? Thanks |
Contributor
@nuno-faria does this look good to you? |
Contributor
|
I merged up to get a fix for the benchmarks |
Contributor
|
run benchmark clickbench_partitioned |
Contributor
|
@nuno-faria and @kumarUjjawal shall we merge this PR? |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing generic-cache-1 (ad6d4e6) to 0fc55d0 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
Contributor
It looks good to me. |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
Also LGTM. |
Contributor
|
woohoo! |
Contributor
|
Thanks @mkleen @kumarUjjawal and @nuno-faria |
This was referenced Jun 18, 2026
Phoenix500526
pushed a commit
to Phoenix500526/datafusion
that referenced
this pull request
Jun 22, 2026
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes None. ## Rationale for this change Follow-up of apache#22613 This moves the cache related tests to `default_cache.rs` where the tests belong to. The tests were previously not moved to have a diff for the review. Now, since apache#22613 is merged we can move the tests to the right location. ## What changes are included in this PR? See above. ## Are these changes tested? This change only moves tests around. ## Are there any user-facing changes? No.
pull Bot
pushed a commit
to buraksenn/datafusion
that referenced
this pull request
Jul 1, 2026
## Which issue does this PR close? - Closes None. ## Rationale for this change Follow up to apache#22613. Cache hits are now supported for all memory-limiting caches. Therefore it makes sense to expose them in the `statistics_cache` function the same way the `metadata_cache` function does it already. ## What changes are included in this PR? - Add cache hits to the `statistics_cache` function - Tests - Adapt documentation for the `statistics_cache` function ## Are these changes tested? Yes. ## Are there any user-facing changes? Yes, the `statistics_cache` function supports now cache hits but no breaking changes.
mkleen
added a commit
to mkleen/datafusion
that referenced
this pull request
Jul 13, 2026
## Which issue does this PR close? - Closes None. ## Rationale for this change Follow up to apache#22613 and apache#23253. Cache hits are now supported for all memory-limiting caches. Therefore it makes sense to expose them in the `list_files_cache` function the same way the `metadata_cache` and `statistics_cache` functions do it already. ## What changes are included in this PR? - Add cache hits to the `list_files_cache` function - Tests - Adapt documentation for the `list_files_cache` function ## Are these changes tested? Yes. ## Are there any user-facing changes? Yes, the `list_files_cache` function supports now cache hits but no breaking changes.
kosiew
pushed a commit
to kosiew/datafusion
that referenced
this pull request
Aug 12, 2026
## Which issue does this PR close? - Closes None. ## Rationale for this change Follow up to apache#22613 and apache#23253. Cache hits are now supported for all memory-limiting caches. Therefore it makes sense to expose them in the `list_files_cache` function the same way the `metadata_cache` and `statistics_cache` functions do it already. ## What changes are included in this PR? - Add cache hits to the `list_files_cache` function - Tests - Adapt documentation for the `list_files_cache` function ## Are these changes tested? Yes. ## Are there any user-facing changes? Yes, the `list_files_cache` function supports now cache hits but no breaking changes.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Which issue does this PR close?
Rationale for this change
This PR introduces a new generic cache with the functionality of file-metadata cache, list-files cache and file-statistics cache into one implementation.
What changes are included in this PR?
Introduce a generic
DefaultCachewith LRU eviction, memory-limit and TTL.Migrate all cache tests to
DefaultCache.Replace file-metadata cache, list-files-cache and file-statistics-cache implementations with the new generic version.
Are these changes tested?
Yes. All existing cache tests are migrated to the new implementation and passing. They had to be slighlty adapted because the new implementation also counts the cache-key for memory accounting which wasn't the case for all previous implementations. The tests are still at the same location to have a diff for reviews.
Are there any user-facing changes?
The traits
FileStatisticsCache,ListFilesCacheandFileMetadataCacheare replaced with the typesCache<TableScopedPath, CachedFileMetadata>,Cache<TableScopedPath, CachedFileList>andCache<Path, CachedFileMetadataEntry>.