viirya commented on PR #5051: URL: https://github.com/apache/datafusion-comet/pull/5051#issuecomment-5441639876
One small accuracy point on the benchmark, separate from the above. `CometInMemoryCacheBenchmark` compares "Comet cache disabled" against "Comet cache enabled", and the `withCachedTable` comment describes the disabled case as *"Spark execution over CometCachedBatch, NOT Spark's own cache format"*. The second half is the important clarification and it's right — both cases read a Comet-written `CometCachedBatch`, so this is not a comparison against Spark's cache format. But "Spark execution" reads as if the disabled case runs the query on Spark operators, and from `cacheConf` that doesn't look right: both cases set `COMET_ENABLED` / `COMET_EXEC_ENABLED = true` and `spark.comet.sparkToColumnar.enabled = true`, and the only flag that flips is `COMET_EXEC_IN_MEMORY_CACHE_ENABLED`. So unless I'm misreading the plan, the two cases differ only at the cache-scan boundary, not in the execution engine above it: - **enabled** → `CometInMemoryTableScan` feeds the Comet operators directly (and `verifyPlan` asserts no `CometSparkColumnarToColumnar`); - **disabled** → Spark's `InMemoryTableScanExec` feeds the same Comet operators through a `CometSparkColumnarToColumnar` bridge — which is a Spark-columnar→Arrow transition whose existence implies the operators above it are still Comet. The `sum(...)` aggregation runs on Comet in both cases; what the benchmark isolates is the cost of that scan-boundary conversion, which is exactly the overhead the PR sets out to remove. That makes the 1.5x / 1.1x a measure of "keep the cached scan native vs fall back to a Spark cache scan + convert", not "Comet vs Spark execution" — a narrower and, I think, more accurate framing than the comment's wording suggests. Worth tightening the `withCachedTable` comment (and the same "Spark execution over CometCachedBatch" phrasing in the PR description) so a reader doesn't take the disabled case as a Spark-execution baseline. If I've misread and the upper operators do fall back to Spark in the disabled case, I'd be glad to be corrected — the plan for the disabled run would settle it. -- 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]
