alamb commented on code in PR #16971:
URL: https://github.com/apache/datafusion/pull/16971#discussion_r2246493406
##########
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:
I studied this code more carefully, and here is an alternate proposal:
What if we made the key, from the cache's perspective, a tuple of `(&Path,
&ObjectMetadata)` and didn't use `extra`
I think that is more in the spirit of this cache where the `ObjectMetadata`
is logically part of the key.
##########
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:
I tried it locally and it seemed possible
What do you think @nuno-faria ?
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
