Repository navigation
Allow support for switching off metadata cache #18362
Description
Currently in datafusion, the CacheManagerConfig has the metadata cache as an option
/// Cache of file-embedded metadata, used to avoid reading it multiple times when processing a
/// data file (e.g., Parquet footer and page metadata).
/// If not provided, the [`CacheManager`] will create a [`DefaultFilesMetadataCache`].
pub file_metadata_cache: Option<Arc<dyn FileMetadataCache>>,
Though as per the doc written above even if we set the file_metadata_cache as None, we will still end up with an DefaultFilesMetadataCache being created on it's own.
I think this makes the API a bit confusing and would like it to atleast honor the cases when we set it to None just like other Config options.
For the same I'm thinking we can initialise the metadata cache object in the CacheManagerConfig in case of default method but in any other case where the user is overriding it, we should honor what's been set instead of creating a cache here in cache_manager:
.file_metadata_cache
.as_ref()
.map(Arc::clone)
.unwrap_or_else(|| {
Arc::new(DefaultFilesMetadataCache::new(config.metadata_cache_limit))
});
Tagging @nuno-faria @alamb who worked on the earlier PRs.
Reacted by Qi Zhu and xudong.w
Activity
alamb commentedon Oct 29, 2025
I agree it would be good to allow setting a null cache manager
nuno-faria commentedon Oct 31, 2025
Also agree, at the time it seemed easier to simply add a default in case it was None.
alchemist51 commentedon Nov 11, 2025
While working on it, we found another issue with MetadataCache object being maintained outside of the Datafusion.
This function in CacheAccessor needs a
muttype of self hence we always need to store the MetadataCache is something like this to get the mut type:Now the current interfaces does not accept the Arc<Mutex> which makes it a bit tricky. We should provide another API in cacheAccessor to remove without mut. @alamb @nuno-faria do you have any thought on how to tackle this in general or why the remove is kepy mut while every other API doesn't need it?
nuno-faria commentedon Nov 11, 2025
@alchemist51 that
mutused in theremoveis something that we also noticed when developing the metadata cache (#17031 (comment)). If I remember correctly there was no need to have it there, but we kept it to not change the API. @alamb suggested a follow up ticket to remove it, and given your issue I think we could now get rid of it.alchemist51 commentedon Nov 12, 2025
Thanks @nuno-faria for the reply. Since as you called out as well it's an public API, should we make another api in CacheAccessor to handle it which I think is kind of not super clean way to do it but could saves us from backward compatibility issues or we want to change the original remove and are fine with an breaking change for the next release?
nuno-faria commentedon Nov 12, 2025
@alchemist51 in my opinion I think the
mutcan be removed rather than implementing a new API. Feel free to open a PR and if anyone has any reservations we can discuss it there.