BlakeOrth commented on code in PR #16971:
URL: https://github.com/apache/datafusion/pull/16971#discussion_r2248565529
##########
datafusion/execution/src/cache/cache_unit.rs:
##########
@@ -157,9 +158,79 @@ impl CacheAccessor<Path, Arc<Vec<ObjectMeta>>> for
DefaultListFilesCache {
}
}
+/// Collected file embedded metadata cache.
+/// The metadata for some file is invalided when the file size or last
modification time have been
+/// changed.
+#[derive(Default)]
+pub struct DefaultFilesMetadataCache {
+ metadata: DashMap<Path, (ObjectMeta, Arc<FileMetadata>)>,
+}
+
+impl CacheAccessor<Path, Arc<FileMetadata>> for DefaultFilesMetadataCache {
+ type Extra = ObjectMeta;
+
+ fn get(&self, _k: &Path) -> Option<Arc<FileMetadata>> {
+ panic!("get in DefaultFilesMetadataCache is not supported, please use
get_with_extra")
+ }
+
+ fn get_with_extra(&self, k: &Path, e: &Self::Extra) ->
Option<Arc<FileMetadata>> {
+ self.metadata
+ .get(k)
+ .map(|s| {
+ let (extra, metadata) = s.value();
+ if extra.size != e.size || extra.last_modified !=
e.last_modified {
+ None
+ } else {
+ Some(Arc::clone(metadata))
+ }
+ })
+ .unwrap_or(None)
+ }
+
+ fn put(&self, _key: &Path, _value: Arc<FileMetadata>) ->
Option<Arc<FileMetadata>> {
+ panic!("put in DefaultFilesMetadataCache is not supported, please use
put_with_extra")
Review Comment:
The `DefaultFileStatistics` cache offers a caller the option of either
checking for object modification or a presumably more performant option where
there's no check (ostensibly this is a situation where the caller can
reasonably assume the underlying files will not change). Is this something that
could reasonably be implemented here, either now or in the future? If yes, it
seems like it's important to ensure the key for the lookup can be reasonably
built with little IO/latency overhead. Presumably this is why the other
`CacheAccessor` implementations rely on `Path` since it's readily available.
I don't necessarily think this needs to be in the initial implementation of
this metadata cache (again, I don't want to slow this PR down if possible), but
it seems like having the option to always rely on the cached metadata when a
caller can reasonably assume underlying objects will not change could be
beneficial. Metadata requests for local files on a SSD/NVMe will be very fast,
however when using remote storage of any sort even the head request to check
modification incurs a relatively significant latency penalty compared to a
blind cache lookup.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
