andygrove commented on code in PR #2498:
URL:
https://github.com/apache/datafusion-ballista/pull/2498#discussion_r4136455962
##########
ballista/scheduler/src/state/session_manager.rs:
##########
@@ -84,3 +86,102 @@ pub fn create_datafusion_context(
Ok(Arc::new(SessionContext::new_with_state(session_state)))
}
+
+/// Wraps `session_builder` so that every session it builds shares one file
+/// statistics cache. [`BallistaCluster::new_memory`] applies this to the
+/// session builder it is given.
+///
+/// The scheduler builds a new session, with its own runtime, for every query.
+/// Planning a scan of a listing table collects statistics by reading the
+/// footer of every file in it, so with a cache per session every job pays for
+/// that again, which on large tables takes seconds. Cached statistics are
+/// checked against the size and modification time from each job's own file
+/// listing, so a file that has changed is read again. That check is why the
+/// listing cache must stay per session: sharing it too would serve stale
+/// statistics, and `COUNT(*)` is answered from them.
+///
+/// Sharing keeps statistics for the scheduler's lifetime instead of one job's.
+/// Entries are keyed by table and store-relative path, and the check compares
+/// neither e-tags nor versions. So a file rewritten in place at the same size,
+/// quickly enough that its modification time does not change, can still be
+/// served stale statistics, and so can a file in another store with the same
+/// path, size and modification time.
Review Comment:
The order makes sense to me. Comparing only the e-tag when both sides have
one also keeps a hit when a file is rewritten with the same bytes, which size
plus modification time would count as a change.
The catch is that the comparison isn't in Ballista. `ListingTable` validates
the cached entry itself by calling DataFusion's
`CachedFileMetadata::is_valid_for`, and `Cache::get` only receives the table
and path. So a cache we pass in never sees the current file's e-tag, and we'd
have to fork the listing table to change it. I filed apache/datafusion#25841 to
do this upstream. Once it lands and we upgrade, the shared cache picks it up
with no change here. 43bb787 points the doc comment at that issue.
--
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]
