andygrove commented on PR #4828: URL: https://github.com/apache/datafusion-comet/pull/4828#issuecomment-5876307638
This is a light fully automated review since there are so many PRs open. 1. After `queue.remove` in `Shard::evict_one` (`native/block-cache/src/sieve.rs:179`), the hand stays on the victim's older neighbor. That is the entry it just cleared, so it goes on the next insert. In SIEVE the hand continues to the victim's newer neighbor. With one shard that fits three blocks, read 0, 1, 2, re-read 0 and 1, then read 3, 4, 5. SIEVE keeps 0 and 1. Here reading 4 evicts 1 and reading 5 evicts 0, while 3 and 4 survive, so under a scan the tier behaves close to FIFO. Could the hand step to `hand - 1` (wrapping to the tail) after the removal, with that sequence as a test? 2. The per-file bookkeeping is never bounded. `BlockCache::intern` (`native/block-cache/src/cache.rs:463`) adds every path an executor reads to `FileTable`, and the entry outlives the file's blocks and sits outside `memoryLimit`. On the driver, `CometFileLocalityManager.assignments` keeps every scanned path, because `pruneStaleAssignments` (`spark/src/main/scala/org/apache/spark/sql/comet/CometFileLocalityManager.scala:144`) is only called from the suite. A Thrift server or a streaming query over new files grows both for the life of the app. Could both be capped or pruned? 3. The cache configs are read from the session conf (`spark/src/main/scala/org/apache/comet/CometExecIterator.scala:287`), but `init_once` (`native/core/src/execution/data_cache.rs:48`) keeps whatever the first plan on each executor carried, while the driver re-reads `enabled` for every scan (`spark/src/main/scala/org/apache/spark/sql/comet/CometNativeScanExec.scala:263`). Turning the cache on with `spark.conf.set` after queries have run gives preferred locations with no cache behind them, and turning it off leaves the executors caching. Could these come from `SparkEnv.get.conf`, the way `spark.executor.cores` does at `CometExecIterator.scala:271`, and be documented as fixed at startup? 4. `memoryLimit` is split over 16 shards (`native/block-cache/src/cache.rs:126`) and a full block costs `blockSize + 64`, so a budget that is an exact multiple of `16 * blockSize` holds one block per shard fewer than expected. `128m` with 4 MiB blocks holds 64 MiB, `512m` with `blockSize=16m` holds 256 MiB, and `64m` holds no full 4 MiB block at all, while `checkValue` (`spark/src/main/scala/org/apache/comet/CometConf.scala:156`) accepts anything above zero. Nothing calls `BlockCache::stats()` in this PR, so a user cannot see it. Could the shard count scale with `memoryLimit / blockSize`, or small budgets be rejected? -- 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]
