Skip to content

Commit 887aa9f

Browse files
fix: preserve ListFilesCache TTL when not set in config (#19401)
## Which issue does this PR close? Closes #19396. ## Rationale for this change Previously, when a `DefaultListFilesCache` was created with a TTL (e.g., `DefaultListFilesCache::new(1024, Some(Duration::from_secs(1)))`) and passed to `CacheManagerConfig` without explicitly setting `list_files_cache_ttl`, the cache's TTL would be unexpectedly unset (overwritten to `None`). This happened because `CacheManager::try_new()` always called `update_cache_ttl(config.list_files_cache_ttl)`, even when the config value was `None`. ## What changes are included in this PR? - Modified `CacheManager::try_new()` to only update the cache's TTL if `config.list_files_cache_ttl` is explicitly set (`Some(value)`). If the config TTL is `None`, the cache's existing TTL is preserved. - Added two test cases: - `test_ttl_preserved_when_not_set_in_config`: Verifies that TTL is preserved when not set in config - `test_ttl_overridden_when_set_in_config`: Verifies that TTL can still be overridden when explicitly set in config ## Are these changes tested? Yes ## Are there any user-facing changes? Yes
1 parent 75d2473 commit 887aa9f

File tree

1 file changed

+78
-1
lines changed

1 file changed

+78
-1
lines changed

datafusion/execution/src/cache/cache_manager.rs

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,10 @@ impl CacheManager {
196196
.inspect(|c| {
197197
// the cache memory limit or ttl might have changed, ensure they are updated
198198
c.update_cache_limit(config.list_files_cache_limit);
199-
c.update_cache_ttl(config.list_files_cache_ttl);
199+
// Only update TTL if explicitly set in config, otherwise preserve the cache's existing TTL
200+
if let Some(ttl) = config.list_files_cache_ttl {
201+
c.update_cache_ttl(Some(ttl));
202+
}
200203
})
201204
.map(Arc::clone);
202205

@@ -350,3 +353,77 @@ impl CacheManagerConfig {
350353
self
351354
}
352355
}
356+
357+
#[cfg(test)]
358+
mod tests {
359+
use super::*;
360+
use crate::cache::DefaultListFilesCache;
361+
362+
/// Test to verify that TTL is preserved when not explicitly set in config.
363+
/// This fixes issue #19396 where TTL was being unset from DefaultListFilesCache
364+
/// when CacheManagerConfig::list_files_cache_ttl was not set explicitly.
365+
#[test]
366+
fn test_ttl_preserved_when_not_set_in_config() {
367+
use std::time::Duration;
368+
369+
// Create a cache with TTL = 1 second
370+
let list_file_cache =
371+
DefaultListFilesCache::new(1024, Some(Duration::from_secs(1)));
372+
373+
// Verify the cache has TTL set initially
374+
assert_eq!(
375+
list_file_cache.cache_ttl(),
376+
Some(Duration::from_secs(1)),
377+
"Cache should have TTL = 1 second initially"
378+
);
379+
380+
// Put cache in config WITHOUT setting list_files_cache_ttl
381+
let config = CacheManagerConfig::default()
382+
.with_list_files_cache(Some(Arc::new(list_file_cache)));
383+
384+
// Create CacheManager from config
385+
let cache_manager = CacheManager::try_new(&config).unwrap();
386+
387+
// Verify TTL is preserved (not unset)
388+
let cache_ttl = cache_manager.get_list_files_cache().unwrap().cache_ttl();
389+
390+
assert!(
391+
cache_ttl.is_some(),
392+
"TTL should be preserved when not set in config. Expected Some(Duration::from_secs(1)), got {cache_ttl:?}"
393+
);
394+
395+
// Verify it's the correct TTL value
396+
assert_eq!(
397+
cache_ttl,
398+
Some(Duration::from_secs(1)),
399+
"TTL should be exactly 1 second"
400+
);
401+
}
402+
403+
/// Test to verify that TTL can still be overridden when explicitly set in config.
404+
#[test]
405+
fn test_ttl_overridden_when_set_in_config() {
406+
use std::time::Duration;
407+
408+
// Create a cache with TTL = 1 second
409+
let list_file_cache =
410+
DefaultListFilesCache::new(1024, Some(Duration::from_secs(1)));
411+
412+
// Put cache in config WITH a different TTL set
413+
let config = CacheManagerConfig::default()
414+
.with_list_files_cache(Some(Arc::new(list_file_cache)))
415+
.with_list_files_cache_ttl(Some(Duration::from_secs(60)));
416+
417+
// Create CacheManager from config
418+
let cache_manager = CacheManager::try_new(&config).unwrap();
419+
420+
// Verify TTL is overridden to the config value
421+
let cache_ttl = cache_manager.get_list_files_cache().unwrap().cache_ttl();
422+
423+
assert_eq!(
424+
cache_ttl,
425+
Some(Duration::from_secs(60)),
426+
"TTL should be overridden to 60 seconds when set in config"
427+
);
428+
}
429+
}

0 commit comments

Comments
 (0)