Feat: DefaultListFilesCache prefix-aware for partition pruning optimization #19298

Yuvraj-cyborg · 2025-12-12T15:50:42Z

Which issue does this PR close?

Closes #19273

Rationale for this change

DefaultListFilesCache currently uses the exact listing path as the cache key. When partition pruning narrows queries to specific partition prefixes (e.g., s3://bucket/table/year=2024/), the cache lookup fails even if a full table listing (s3://bucket/table/) was previously cached. This leads to redundant object store calls and duplicate cache entries for different partition filters on the same table.

What changes are included in this PR?

Updated ListFilesCache trait to use Extra = Option<Path> (partition prefix) instead of ObjectMeta
Added get_with_prefix(table_base, prefix, now) method to DefaultListFilesCache that:
- Uses the table base path as a stable cache key
- Optionally filters cached results by a partition prefix
- Handles TTL expiration checks
Updated list_with_cache in url.rs to:
- Always use the table base path as the cache key
- Compute the relative prefix between the listing URL and table base
- Always cache full table listings to ensure complete data is available for subsequent partition queries
- Added compute_relative_prefix helper function

Are these changes tested?

Yes. Six dedicated unit tests validate prefix-aware cache behavior:

test_prefix_aware_cache_hit - filters cached results by prefix
test_prefix_aware_cache_no_filter_returns_all - returns all files when no prefix specified
test_prefix_aware_cache_miss_no_entry - handles cache misses correctly
test_prefix_aware_cache_no_matching_files - returns empty when no files match prefix
test_prefix_aware_nested_partitions - handles nested partition paths (e.g., year=2024/month=01/)
test_prefix_aware_different_tables - ensures different tables have isolated cache entries

Are there any user-facing changes?

No direct API changes. Users will see improved cache efficiency when querying partitioned tables - partition-pruned queries can now be served from cached full-table listings, reducing object store calls.

Yuvraj-cyborg · 2025-12-12T15:57:20Z

@BlakeOrth Could you check this, if this is the approach you were suggesting, table's base path is key and extra being the prefix ?

BlakeOrth

Thanks for looking into this! I do think this is generally what I had in mind, and I'm relieved to see the actual implementation didn't seem to have too many surprises. I reviewed this code carefully and have left a few comments.

datafusion/datasource/src/url.rs

datafusion/execution/src/cache/list_files_cache.rs

datafusion/execution/src/cache/cache_manager.rs

BlakeOrth

Thanks for this most recent set of changes. I think this is looking even better now!

datafusion/datasource/src/url.rs

datafusion/execution/src/cache/cache_manager.rs

Yuvraj-cyborg · 2025-12-13T06:22:15Z

@BlakeOrth I did the changes you asked but for the last one , but if we change Option<Path> to Option<&Path> , we need to change CacheAccessor trait's Extra type to be a reference and there can be complexity in lifetimes...

Again just an opinion
If you think that's a better architecture then would change it !

Guess I as well need to spend more time with the codebase .

Yuvraj-cyborg · 2025-12-15T15:49:21Z

Hey @Jefffrey @BlakeOrth could you review this one !!

BlakeOrth

Sorry for the short delay on the review, I think this is looking good to me now. Thank you! My approval doesn't have any power here so we'll still need some time from a maintainer.

cc @alamb

BlakeOrth · 2025-12-15T17:48:40Z

but if we change Option<Path> to Option<&Path> , we need to change CacheAccessor trait's Extra type to be a reference and there can be complexity in lifetimes...

Yes, I thought this might end up happening. I don't think it's a big deal, but I do think it was worth at least exploring. Thanks for taking the time to look into it!

alamb

Thank you @Yuvraj-cyborg and @BlakeOrth -- I took a look at this and it looks great to me

It would be nicer to reduce the verbosity of the testing harness, but I also think we could do it as a follow on PR

datafusion/datasource/src/url.rs

…-else style

…s, remove TTL from tests

Yuvraj-cyborg · 2025-12-16T19:18:18Z

@alamb I've made the changes you asked for !

alamb · 2025-12-16T19:26:47Z

datafusion/datasource/src/url.rs

        }
+
+        /// Create a MockSession with a custom RuntimeEnv (for cache testing)
+        fn with_runtime_env(runtime_env: Arc<RuntimeEnv>) -> Self {


alamb · 2025-12-17T21:12:28Z

Thanks again @Yuvraj-cyborg

github-actions bot added execution Related to the execution crate datasource Changes to the datasource crate labels Dec 12, 2025

BlakeOrth reviewed Dec 12, 2025

View reviewed changes

datafusion/datasource/src/url.rs Outdated Show resolved Hide resolved

datafusion/datasource/src/url.rs Show resolved Hide resolved

datafusion/execution/src/cache/list_files_cache.rs Outdated Show resolved Hide resolved

datafusion/execution/src/cache/cache_manager.rs Show resolved Hide resolved

Yuvraj-cyborg force-pushed the files-cache branch from 47e1bcf to ce365cc Compare December 12, 2025 20:23

BlakeOrth reviewed Dec 12, 2025

View reviewed changes

Yuvraj-cyborg force-pushed the files-cache branch from ce365cc to 5614873 Compare December 13, 2025 06:16

Yuvraj-cyborg requested a review from BlakeOrth December 13, 2025 19:48

BlakeOrth approved these changes Dec 15, 2025

View reviewed changes

alamb approved these changes Dec 16, 2025

View reviewed changes

datafusion/datasource/src/url.rs Outdated Show resolved Hide resolved

Yuvraj-cyborg force-pushed the files-cache branch from 5614873 to 2aa1685 Compare December 16, 2025 19:11

Yuvraj-cyborg added 5 commits December 17, 2025 00:43

feat: prefix aware cache impl

35b8a05

Apply rustfmt

41b5792

Address review feedback: simplify API, add equivalence tests, use let…

db9785e

…-else style

fix: keep prefix naming, remove empty path handling, sort test result…

9826ca7

…s, remove TTL from tests

Consolidate MockSession and MockSessionWithRuntime into single struct

a1a8ea0

Yuvraj-cyborg force-pushed the files-cache branch from 2aa1685 to a1a8ea0 Compare December 16, 2025 19:17

alamb reviewed Dec 16, 2025

View reviewed changes

alamb added this pull request to the merge queue Dec 17, 2025

Merged via the queue into apache:main with commit 1e4bd75 Dec 17, 2025
27 checks passed

Feat: DefaultListFilesCache prefix-aware for partition pruning optimization #19298

Feat: DefaultListFilesCache prefix-aware for partition pruning optimization #19298

Uh oh!

Conversation

Yuvraj-cyborg commented Dec 12, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Yuvraj-cyborg commented Dec 12, 2025

Uh oh!

BlakeOrth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BlakeOrth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yuvraj-cyborg commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yuvraj-cyborg commented Dec 15, 2025

Uh oh!

BlakeOrth left a comment

Choose a reason for hiding this comment

Uh oh!

BlakeOrth commented Dec 15, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yuvraj-cyborg commented Dec 16, 2025

Uh oh!

alamb Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

alamb commented Dec 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Yuvraj-cyborg commented Dec 13, 2025 •

edited

Loading