andygrove commented on PR #2498: URL: https://github.com/apache/datafusion-ballista/pull/2498#issuecomment-5875419469
@milenkovicm on the rest of your review: **SF1000.** I haven't measured SF1000 with the fix yet. The closest data point is the SF100 files linked 10 times (320 files per table), which approximates SF1000's footer volume. There, Q21 planning went from 1542 ms to 10 ms once `lineitem` had been scanned. The cost is linear in the number of files, so I'd expect SF1000 to drop from over 4 s to tens of ms after the first query on each table. That first query still pays the full cost. **The listing cache.** Two DataFusion caches are involved. The listing cache maps a table's path to the files under it, including each file's size and modification time. A hit is used as is, and by default entries never expire. The statistics cache maps each file to its statistics, and a hit is only used if the size and modification time match the current listing. So each job lists the files itself, and cached statistics are checked against that fresh listing. If the listing cache were shared too, a later job would get the old listing and the old statistics would pass the check. `COUNT(*)` is answered from statistics, so it would return the old count. New files wouldn't show up either. `test_in_memory_sessions_reread_changed_files` fails with the stale count if the listing cache is shared. **Session ID.** With the default builder every job does get a new random ID. But `ctx.session_id()` becomes the job's session ID, which is sent with each task, and executors key their runtime cache on it. A builder can set it, like the one `new_standalone_scheduler_from_state` uses, which hands out the client's state. So I'd rather the wrapper not change it. With the early return @comphead suggested, sessions that already have the shared cache aren't rebuilt at all. **A `static` cache in `default_session_builder_with_cache`.** The binary never calls `default_session_builder` (see the inline thread), and a `static` would share one cache across every scheduler in the process. So I went with `new_memory`. -- 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]
