You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Yuvraj-cyborg (via GitHub)" <gi...@apache.org> on 2025/12/12 15:50:45 UTC
[PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Yuvraj-cyborg opened a new pull request, #19298:
URL: https://github.com/apache/datafusion/pull/19298
## 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "Yuvraj-cyborg (via GitHub)" <gi...@apache.org>.
Yuvraj-cyborg commented on PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#issuecomment-3656359154
Hey @Jefffrey @BlakeOrth could you review this one !!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#discussion_r2624429652
##########
datafusion/datasource/src/url.rs:
##########
@@ -908,6 +1141,83 @@ mod tests {
}
}
+ /// MockSession variant that accepts a custom RuntimeEnv (for cache testing)
Review Comment:
I think this is a lot of boiler plate code just to add the ability to change the runtime env
Would you be willing to just add a `Option<RuntimeEnv>` field to the existing `MockSession`?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "Yuvraj-cyborg (via GitHub)" <gi...@apache.org>.
Yuvraj-cyborg commented on code in PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#discussion_r2624434254
##########
datafusion/datasource/src/url.rs:
##########
@@ -908,6 +1141,83 @@ mod tests {
}
}
+ /// MockSession variant that accepts a custom RuntimeEnv (for cache testing)
Review Comment:
@alamb mm sure !
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "BlakeOrth (via GitHub)" <gi...@apache.org>.
BlakeOrth commented on code in PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#discussion_r2615148301
##########
datafusion/datasource/src/url.rs:
##########
@@ -324,33 +324,105 @@ impl ListingTableUrl {
}
}
+/// Lists files with cache support, using prefix-aware lookups.
+///
+/// # Arguments
+/// * `ctx` - The session context
+/// * `store` - The object store to list from
+/// * `full_prefix` - The full prefix to list (table_base + partition prefix)
+/// * `table_base_path` - The table's base path (the stable cache key)
+///
+/// # Cache Behavior :
+/// The cache key is always `table_base_path`. When a partition-specific listing
+/// is requested (full_prefix includes partition path), the cache:
+/// - Looks up `table_base_path` in the cache
+/// - Filters results to match `full_prefix`
+/// - Returns filtered results without a storage call
+///
+/// On cache miss, the full table is always listed and cached, ensuring
+/// subsequent partition queries can be served from cache.
async fn list_with_cache<'b>(
ctx: &'b dyn Session,
store: &'b dyn ObjectStore,
- prefix: &Path,
+ full_prefix: &Path,
+ table_base_path: &Path,
) -> Result<BoxStream<'b, Result<ObjectMeta>>> {
match ctx.runtime_env().cache_manager.get_list_files_cache() {
None => Ok(store
- .list(Some(prefix))
+ .list(Some(full_prefix))
.map(|res| res.map_err(|e| DataFusionError::ObjectStore(Box::new(e))))
.boxed()),
Some(cache) => {
- let vec = if let Some(res) = cache.get(prefix) {
- debug!("Hit list all files cache");
+ // Compute the relative prefix (partition path relative to table base)
+ let relative_prefix = compute_relative_prefix(table_base_path, full_prefix);
Review Comment:
I'm feeling a bit confused with the structure of this code. The calling method already has the knowledge of the table's base path vs a user's provided prefix, and it concatenates them into a full listing path here:
https://github.com/Yuvraj-cyborg/datafusion/blob/47e1bcf9cb91b83fe0b46d7bc0cfa232fa60e2cd/datafusion/datasource/src/url.rs#L248-L254
Rather than performing that concatenation and then deconstructing the path, can we just pass in `table_base_path` and `prefix`?
##########
datafusion/datasource/src/url.rs:
##########
@@ -324,33 +324,105 @@ impl ListingTableUrl {
}
}
+/// Lists files with cache support, using prefix-aware lookups.
+///
+/// # Arguments
+/// * `ctx` - The session context
+/// * `store` - The object store to list from
+/// * `full_prefix` - The full prefix to list (table_base + partition prefix)
+/// * `table_base_path` - The table's base path (the stable cache key)
+///
+/// # Cache Behavior :
+/// The cache key is always `table_base_path`. When a partition-specific listing
+/// is requested (full_prefix includes partition path), the cache:
+/// - Looks up `table_base_path` in the cache
+/// - Filters results to match `full_prefix`
+/// - Returns filtered results without a storage call
+///
+/// On cache miss, the full table is always listed and cached, ensuring
+/// subsequent partition queries can be served from cache.
async fn list_with_cache<'b>(
ctx: &'b dyn Session,
store: &'b dyn ObjectStore,
- prefix: &Path,
+ full_prefix: &Path,
+ table_base_path: &Path,
) -> Result<BoxStream<'b, Result<ObjectMeta>>> {
match ctx.runtime_env().cache_manager.get_list_files_cache() {
None => Ok(store
- .list(Some(prefix))
+ .list(Some(full_prefix))
.map(|res| res.map_err(|e| DataFusionError::ObjectStore(Box::new(e))))
.boxed()),
Some(cache) => {
Review Comment:
Given how critical this functionality is for correct results from a listing table, and the fact that this method now has additional complexity, I think some additional tests that exercise the path for `Some(cache)` would be nice. In my mind it would be good to assert the results from the `Some(cache)` code path and the `None` code path always generate equivalent outputs.
##########
datafusion/execution/src/cache/list_files_cache.rs:
##########
@@ -172,20 +172,73 @@ impl DefaultListFilesCacheState {
}
}
- /// Returns the respective entry from the cache, if it exists and the entry
- /// has not expired by `now`.
+ /// Performs a prefix-aware cache lookup.
///
- /// If the entry exists it becomes the most recently used. If the entry has expired it is
- /// removed from the cache
- fn get(&mut self, key: &Path, now: Instant) -> Option<Arc<Vec<ObjectMeta>>> {
- let entry = self.lru_queue.get(key)?;
+ /// # Arguments
+ /// * `table_base` - The table's base path (the cache key)
+ /// * `prefix` - Optional prefix filter relative to the table base path
+ /// * `now` - Current time for expiration checking
+ ///
+ /// # Behavior
+ /// - Fetches the cache entry for `table_base`
+ /// - If `prefix` is `Some`, filters results to only files matching `table_base/prefix`
+ /// - Returns the (potentially filtered) results
+ ///
+ /// # Example
+ /// ```text
+ /// get_with_prefix("my_table", Some("a=1"), now)
+ /// → Fetch cache entry for "my_table"
+ /// → Filter to files matching "my_table/a=1/*"
+ /// → Return filtered results
+ /// ```
+ fn get_with_prefix(
+ &mut self,
+ table_base: &Path,
+ prefix: Option<&Path>,
+ now: Instant,
+ ) -> Option<Arc<Vec<ObjectMeta>>> {
+ let entry = self.lru_queue.get(table_base)?;
- match entry.expires {
- Some(exp) if now > exp => {
- self.remove(key);
- None
+ // Check expiration
+ if let Some(exp) = entry.expires
+ && now > exp
+ {
+ self.remove(table_base);
+ return None;
+ }
+
+ match prefix {
+ None => {
+ // No prefix filter, return all files
+ Some(Arc::clone(&entry.metas))
+ }
+ Some(prefix) => {
+ // Build the full prefix path: table_base/prefix
+ let full_prefix = if table_base.as_ref().is_empty() {
Review Comment:
Style nit -- I think this code would format a bit better using a `let-else` statement rather than a `match`:
```rust
let Some(prefix) = prefix else {
return Some(Arc::clone(&entry.metas))
}
let full_prefix = ...
```
##########
datafusion/execution/src/cache/cache_manager.rs:
##########
@@ -64,9 +64,21 @@ pub struct FileStatisticsCacheEntry {
/// command on the local filesystem. This operation can be expensive,
/// especially when done over remote object stores.
///
+/// The cache key is always the table's base path, ensuring a stable cache key.
+/// The `Extra` type is `Option<Path>`, representing an optional prefix filter
+/// (relative to the table base path) for partition-aware lookups.
+///
+/// When `get_with_extra(key, Some(prefix))` is called:
+/// - The cache entry for `key` (table base path) is fetched
+/// - Results are filtered to only include files matching `key/prefix`
+/// - Filtered results are returned without making a storage call
+///
+/// This enables efficient partition pruning: a single cached listing of the
+/// full table can serve queries for any partition subset.
+///
/// See [`crate::runtime_env::RuntimeEnv`] for more details.
pub trait ListFilesCache:
- CacheAccessor<Path, Arc<Vec<ObjectMeta>>, Extra = ObjectMeta>
+ CacheAccessor<Path, Arc<Vec<ObjectMeta>>, Extra = Option<Path>>
Review Comment:
Does having `Extra` as `Option<Path>` as opposed to just `Path` buy us anything here?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#issuecomment-3667186511
Thanks again @Yuvraj-cyborg
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "Yuvraj-cyborg (via GitHub)" <gi...@apache.org>.
Yuvraj-cyborg commented on PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#issuecomment-3649034950
@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 think that's a better architecture then would change it !
Guess I as well need to spend more time with the codebase .
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "BlakeOrth (via GitHub)" <gi...@apache.org>.
BlakeOrth commented on PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#issuecomment-3656880368
> 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!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #19298:
URL: https://github.com/apache/datafusion/pull/19298
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "BlakeOrth (via GitHub)" <gi...@apache.org>.
BlakeOrth commented on code in PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#discussion_r2615769167
##########
datafusion/datasource/src/url.rs:
##########
@@ -826,6 +803,190 @@ mod tests {
Ok(())
}
+ /// Tests that the cached code path produces identical results to the non-cached path.
+ ///
+ /// This is critical: the cache is a transparent optimization, so both paths
+ /// MUST return the same files in the same order.
+ #[tokio::test]
+ async fn test_cache_path_equivalence() -> Result<()> {
+ use datafusion_execution::runtime_env::RuntimeEnvBuilder;
+ use std::time::Duration;
+
+ let store = MockObjectStore {
+ in_mem: object_store::memory::InMemory::new(),
+ forbidden_paths: vec![],
+ };
+
+ // Create test files with partition-style paths
+ create_file(&store, "/table/year=2023/data1.parquet").await;
+ create_file(&store, "/table/year=2023/month=01/data2.parquet").await;
+ create_file(&store, "/table/year=2024/data3.parquet").await;
+ create_file(&store, "/table/year=2024/month=06/data4.parquet").await;
+ create_file(&store, "/table/year=2024/month=12/data5.parquet").await;
+
+ // Session WITHOUT cache
+ let session_no_cache = MockSession::new();
+
+ // Session WITH cache - use RuntimeEnvBuilder with cache limits
+ let runtime_with_cache = RuntimeEnvBuilder::new()
+ .with_object_list_cache_limit(1024 * 1024) // 1MB limit
+ .with_object_list_cache_ttl(Duration::from_secs(300))
Review Comment:
Since this test isn't really relying on entry expiration this can probably left at its default (`None`) to entirely avoid potential test timing issues.
##########
datafusion/datasource/src/url.rs:
##########
@@ -826,6 +803,190 @@ mod tests {
Ok(())
}
+ /// Tests that the cached code path produces identical results to the non-cached path.
+ ///
+ /// This is critical: the cache is a transparent optimization, so both paths
+ /// MUST return the same files in the same order.
+ #[tokio::test]
+ async fn test_cache_path_equivalence() -> Result<()> {
+ use datafusion_execution::runtime_env::RuntimeEnvBuilder;
+ use std::time::Duration;
+
+ let store = MockObjectStore {
+ in_mem: object_store::memory::InMemory::new(),
+ forbidden_paths: vec![],
+ };
+
+ // Create test files with partition-style paths
+ create_file(&store, "/table/year=2023/data1.parquet").await;
+ create_file(&store, "/table/year=2023/month=01/data2.parquet").await;
+ create_file(&store, "/table/year=2024/data3.parquet").await;
+ create_file(&store, "/table/year=2024/month=06/data4.parquet").await;
+ create_file(&store, "/table/year=2024/month=12/data5.parquet").await;
+
+ // Session WITHOUT cache
+ let session_no_cache = MockSession::new();
+
+ // Session WITH cache - use RuntimeEnvBuilder with cache limits
+ let runtime_with_cache = RuntimeEnvBuilder::new()
+ .with_object_list_cache_limit(1024 * 1024) // 1MB limit
+ .with_object_list_cache_ttl(Duration::from_secs(300))
+ .build_arc()?;
+ let session_with_cache = MockSessionWithRuntime::new(runtime_with_cache);
+
+ // Test cases: (url, prefix, description)
+ let test_cases = vec![
+ ("/table/", None, "full table listing"),
+ (
+ "/table/",
+ Some(Path::from("year=2023")),
+ "single partition filter",
+ ),
+ (
+ "/table/",
+ Some(Path::from("year=2024")),
+ "different partition filter",
+ ),
+ (
+ "/table/",
+ Some(Path::from("year=2024/month=06")),
+ "nested partition filter",
+ ),
+ (
+ "/table/",
+ Some(Path::from("year=2025")),
+ "non-existent partition",
+ ),
+ ];
+
+ for (url_str, prefix, description) in test_cases {
+ let url = ListingTableUrl::parse(url_str)?;
+
+ // Get results WITHOUT cache
+ let results_no_cache: Vec<String> = url
+ .list_prefixed_files(&session_no_cache, &store, prefix.clone(), "parquet")
+ .await?
+ .try_collect::<Vec<_>>()
+ .await?
+ .into_iter()
+ .map(|m| m.location.to_string())
+ .collect();
+
+ // Get results WITH cache (first call - cache miss)
+ let results_with_cache_miss: Vec<String> = url
+ .list_prefixed_files(
+ &session_with_cache,
+ &store,
+ prefix.clone(),
+ "parquet",
+ )
+ .await?
+ .try_collect::<Vec<_>>()
+ .await?
+ .into_iter()
+ .map(|m| m.location.to_string())
+ .collect();
+
+ // Get results WITH cache (second call - cache hit)
+ let results_with_cache_hit: Vec<String> = url
+ .list_prefixed_files(&session_with_cache, &store, prefix, "parquet")
+ .await?
+ .try_collect::<Vec<_>>()
+ .await?
+ .into_iter()
+ .map(|m| m.location.to_string())
+ .collect();
+
+ // All three should be identical
+ assert_eq!(
+ results_no_cache, results_with_cache_miss,
+ "Cache miss path should match non-cached path for: {description}"
+ );
+ assert_eq!(
+ results_no_cache, results_with_cache_hit,
+ "Cache hit path should match non-cached path for: {description}"
+ );
+ }
+
+ Ok(())
+ }
+
+ /// Tests that partition queries can be served from a cached full-table listing
+ #[tokio::test]
+ async fn test_cache_serves_partition_from_full_listing() -> Result<()> {
+ use datafusion_execution::runtime_env::RuntimeEnvBuilder;
+ use std::time::Duration;
+
+ let store = MockObjectStore {
+ in_mem: object_store::memory::InMemory::new(),
+ forbidden_paths: vec![],
+ };
+
+ // Create test files
+ create_file(&store, "/sales/region=US/q1.parquet").await;
+ create_file(&store, "/sales/region=US/q2.parquet").await;
+ create_file(&store, "/sales/region=EU/q1.parquet").await;
+
+ // Create session with cache - use RuntimeEnvBuilder with cache limits
+ let runtime = RuntimeEnvBuilder::new()
+ .with_object_list_cache_limit(1024 * 1024) // 1MB limit
+ .with_object_list_cache_ttl(Duration::from_secs(300))
Review Comment:
The same order criteria and default TTL apply to this test as well
##########
datafusion/datasource/src/url.rs:
##########
@@ -329,36 +331,46 @@ impl ListingTableUrl {
/// # Arguments
/// * `ctx` - The session context
/// * `store` - The object store to list from
-/// * `full_prefix` - The full prefix to list (table_base + partition prefix)
/// * `table_base_path` - The table's base path (the stable cache key)
+/// * `partition_prefix` - Optional partition prefix relative to table base
///
-/// # Cache Behavior :
+/// # Cache Behavior:
/// The cache key is always `table_base_path`. When a partition-specific listing
-/// is requested (full_prefix includes partition path), the cache:
+/// is requested via `partition_prefix`, the cache:
/// - Looks up `table_base_path` in the cache
-/// - Filters results to match `full_prefix`
+/// - Filters results to match `table_base_path/partition_prefix`
/// - Returns filtered results without a storage call
///
/// On cache miss, the full table is always listed and cached, ensuring
/// subsequent partition queries can be served from cache.
async fn list_with_cache<'b>(
ctx: &'b dyn Session,
store: &'b dyn ObjectStore,
- full_prefix: &Path,
table_base_path: &Path,
+ partition_prefix: Option<&Path>,
) -> Result<BoxStream<'b, Result<ObjectMeta>>> {
+ // Build the full listing path (table_base + partition_prefix)
+ let full_prefix = match partition_prefix {
+ Some(prefix) if !prefix.as_ref().is_empty() => {
Review Comment:
If we look at the documentation for an `object_store::Path` I don't think we need to handle empty path segments, because they aren't considered a valid `Path`
https://docs.rs/object_store/0.12.4/object_store/path/struct.Path.html
> - Paths do not contain empty path segments
##########
datafusion/datasource/src/url.rs:
##########
@@ -239,13 +239,14 @@ impl ListingTableUrl {
&'a self,
ctx: &'a dyn Session,
store: &'a dyn ObjectStore,
- prefix: Option<Path>,
+ partition_prefix: Option<Path>,
Review Comment:
A small naming nit here -- In the context of this PR I think this name change makes sense, however the notion of a "partition" is pretty specific to the `ListingTable` and this method is a more general helper that can be used for operations not related to the `ListingTable`. I think keeping the name as `prefix` is more generally applicable for users of the method outside the context of the `ListingTable`.
##########
datafusion/datasource/src/url.rs:
##########
@@ -826,6 +803,190 @@ mod tests {
Ok(())
}
+ /// Tests that the cached code path produces identical results to the non-cached path.
+ ///
+ /// This is critical: the cache is a transparent optimization, so both paths
+ /// MUST return the same files in the same order.
Review Comment:
This is only partially true. The returned order of the files does not matter. In fact, the order of items returned by `ObjectStore::list` is not guaranteed:
https://docs.rs/object_store/0.12.4/object_store/trait.ObjectStore.html#tymethod.list
To prevent this test from potentially being flaky (I _doubt_ it really matters for a local object store, but better safe than sorry) we should probably collect and then sort the results rather than make hard assertions on the return order of the items.
##########
datafusion/execution/src/cache/cache_manager.rs:
##########
@@ -64,9 +64,21 @@ pub struct FileStatisticsCacheEntry {
/// command on the local filesystem. This operation can be expensive,
/// especially when done over remote object stores.
///
+/// The cache key is always the table's base path, ensuring a stable cache key.
+/// The `Extra` type is `Option<Path>`, representing an optional prefix filter
+/// (relative to the table base path) for partition-aware lookups.
+///
+/// When `get_with_extra(key, Some(prefix))` is called:
+/// - The cache entry for `key` (table base path) is fetched
+/// - Results are filtered to only include files matching `key/prefix`
+/// - Filtered results are returned without making a storage call
+///
+/// This enables efficient partition pruning: a single cached listing of the
+/// full table can serve queries for any partition subset.
+///
/// See [`crate::runtime_env::RuntimeEnv`] for more details.
pub trait ListFilesCache:
- CacheAccessor<Path, Arc<Vec<ObjectMeta>>, Extra = ObjectMeta>
+ CacheAccessor<Path, Arc<Vec<ObjectMeta>>, Extra = Option<Path>>
Review Comment:
To be clear, this was more of an open question I had while reading this as opposed to a request for changes! I'm perfectly fine with `Option<Path>` (maybe we can do `Option<&Path>` to avoid unnecessary clones?) if we all think it's the right idea.
1. I don't think we need to worry about empty string handling throughout this implementation because `object_store::Path` should already protect against that (although it's probably worth validating this assertion). https://docs.rs/object_store/0.12.4/object_store/path/struct.Path.html
2. I agree, but the user API of this cache already has `get(key: &Path)` which represents an unfiltered query. It's also probably beneficial to keep in mind this cache interface does not just apply to the `ListingTable`, so it may be best to think of the implementation in more general "object store" terms instead of "query" terms (e.g. a general path "prefix" instead of a "path partition")
I think the code internally should stay mostly the same so we can functionally continue delegating `get(key)` as `get_with_extra(key, None)`. I guess the question is mostly how we want the code to present to a user at the callsite and if there's value in forcing user's who do not have a prefix to use `get(key)` (to explicitly show a lack of prefix filter) over `get_with_extra(key, None)`.
Again, I don't feel too strongly about this, but at least wanted to bring it up for some discussion!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "Yuvraj-cyborg (via GitHub)" <gi...@apache.org>.
Yuvraj-cyborg commented on code in PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#discussion_r2615502085
##########
datafusion/execution/src/cache/cache_manager.rs:
##########
@@ -64,9 +64,21 @@ pub struct FileStatisticsCacheEntry {
/// command on the local filesystem. This operation can be expensive,
/// especially when done over remote object stores.
///
+/// The cache key is always the table's base path, ensuring a stable cache key.
+/// The `Extra` type is `Option<Path>`, representing an optional prefix filter
+/// (relative to the table base path) for partition-aware lookups.
+///
+/// When `get_with_extra(key, Some(prefix))` is called:
+/// - The cache entry for `key` (table base path) is fetched
+/// - Results are filtered to only include files matching `key/prefix`
+/// - Filtered results are returned without making a storage call
+///
+/// This enables efficient partition pruning: a single cached listing of the
+/// full table can serve queries for any partition subset.
+///
/// See [`crate::runtime_env::RuntimeEnv`] for more details.
pub trait ListFilesCache:
- CacheAccessor<Path, Arc<Vec<ObjectMeta>>, Extra = ObjectMeta>
+ CacheAccessor<Path, Arc<Vec<ObjectMeta>>, Extra = Option<Path>>
Review Comment:
Fixed the previous change suggestions but this one I'm still in doubt because of :
1. empty string behaviours.
2. In partition pruning, the absence of a filter (query full table) is fundamentally different from filtering by a specific partition.
If these aren't the concerns then I can proceed to remove the ```Option<> ```
cc: @BlakeOrth
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "Yuvraj-cyborg (via GitHub)" <gi...@apache.org>.
Yuvraj-cyborg commented on PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#issuecomment-3662021284
@alamb I've made the changes you asked for !
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#discussion_r2624476496
##########
datafusion/datasource/src/url.rs:
##########
@@ -906,6 +1139,14 @@ mod tests {
runtime_env: Arc::new(RuntimeEnv::default()),
}
}
+
+ /// Create a MockSession with a custom RuntimeEnv (for cache testing)
+ fn with_runtime_env(runtime_env: Arc<RuntimeEnv>) -> Self {
Review Comment:
👍
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Feat: DefaultListFilesCache prefix-aware for partition pruning optimization [datafusion]
Posted by "Yuvraj-cyborg (via GitHub)" <gi...@apache.org>.
Yuvraj-cyborg commented on PR #19298:
URL: https://github.com/apache/datafusion/pull/19298#issuecomment-3647152956
@BlakeOrth Could you check this, if this is the approach you were suggesting, table's base path is key and extra being the prefix ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org