villebro commented on code in PR #25858:
URL: https://github.com/apache/datafusion/pull/25858#discussion_r4137159847
##########
datafusion/common/src/rounding.rs:
##########
@@ -254,7 +254,7 @@ where
}
}
_ => {}
- };
+ }
Review Comment:
Hmm, I see we're not linting the code for Windows target (I see context in
https://github.com/apache/datafusion/issues/13726, but it's still a shame we
can have incorrect formatting here and there, especially if Windows is
explicitly supported).
##########
datafusion/execution/src/cache/cache_manager.rs:
##########
@@ -124,19 +127,36 @@ impl CachedFileMetadata {
/// Check if this cached entry is still valid for the given metadata.
///
/// Returns true if the file size, last modified time, and schema match.
+ /// ETag and version must also match when present in both metadata values.
pub fn is_valid_for(
&self,
current_meta: &ObjectMeta,
current_schema_fingerprint: &Arc<SchemaFingerprint>,
) -> bool {
- self.meta.size == current_meta.size
- && self.meta.last_modified == current_meta.last_modified
+ file_metadata_matches(&self.meta, current_meta)
&& (Arc::ptr_eq(&self.schema_fingerprint,
current_schema_fingerprint)
|| self.schema_fingerprint.as_ref()
== current_schema_fingerprint.as_ref())
}
}
+/// Size and modification time must always match. Some stores or requests omit
+/// object identifiers, so compare each only when both metadata values provide
it.
+fn file_metadata_matches(cached: &ObjectMeta, current: &ObjectMeta) -> bool {
+ cached.size == current.size
+ && cached.last_modified == current.last_modified
+ && cached
+ .e_tag
+ .as_ref()
+ .zip(current.e_tag.as_ref())
+ .is_none_or(|(cached, current)| cached == current)
+ && cached
+ .version
+ .as_ref()
+ .zip(current.version.as_ref())
+ .is_none_or(|(cached, current)| cached == current)
+}
+
Review Comment:
I made a comment on
https://github.com/apache/datafusion-ballista/pull/2498#discussion_r4125396205
suggesting short circuiting in the following order:
- if both sides have etag, just compare those (minimizes cache miss if the
content is byte identical)
- if both sides have version, compare those (can cache miss if byte
identical change was pushed)
- last resort: modification timestamp + size
--
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]
