Allow support for switching off metadata cache

[alchemist51]

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.

[alamb]

I agree it would be good to allow setting a null cache manager

[nuno-faria]

Also agree, at the time it seemed easier to simply add a default in case it was None.

[alchemist51]

While working on it, we found another issue with MetadataCache object being maintained outside of the Datafusion.

fn remove(&mut self, k: &K) -> Option<V>;

This function in CacheAccessor needs a mut type of self hence we always need to store the MetadataCache is something like this to get the mut type:

Arc<Mutex<DefaultMetadataCache> 

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]

@alchemist51 that mut used in the remove is 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]

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]

@alchemist51 in my opinion I think the mut can 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.