andygrove commented on PR #5051:
URL:
https://github.com/apache/datafusion-comet/pull/5051#issuecomment-5443347184
@viirya you read it correctly and I had it wrong. Fixed in 7e19dff20.
Here is the plan for the disabled run, which settles it:
```
*(1) CometColumnarToRow
+- CometHashAggregate [sum#265L, sum#266L, sum#267L], [Final], [sum(id#0L),
sum(k#1L), sum(v#2L)]
+- CometExchange SinglePartition, ENSURE_REQUIREMENTS, CometNativeShuffle
+- CometHashAggregate [id#0L, k#1L, v#2L], [Partial],
[partial_sum(id#0L), ...]
+- CometSparkColumnarToColumnar
+- Scan In-memory table bench_cache [id#0L, k#1L, v#2L]
+- InMemoryRelation [...], StorageLevel(disk, memory,
deserialized, 1 replicas)
```
The aggregation is `CometHashAggregate` on both sides, over `CometExchange`,
and the only difference is that `CometSparkColumnarToColumnar` + `Scan
In-memory table` becomes `CometInMemoryTableScan`. So your reading of
`cacheConf` was right and the "Spark execution" half of that comment was simply
wrong — the correct framing is the narrower one you gave: keep the cached scan
native versus fall back to a Spark cache scan and convert.
What changed:
* The case labels are now `Spark cache scan + CometSparkColumnarToColumnar`
and `CometInMemoryTableScan`, so the comparison is legible from the benchmark
output alone rather than depending on a comment.
* The `withCachedTable` comment states which flag moves and which does not,
and keeps the separate point that neither case is a baseline for Spark's own
cache format.
* The PR description is corrected the same way. I had already fixed one
wrong framing there — presenting the numbers as a speedup over Spark's cache —
and replaced it with a second wrong one, so the description now says both
explicitly.
One addition beyond what you asked for: `verifyPlan` now asserts
`CometSparkColumnarToColumnar` is present in the disabled case, not just that
`CometInMemoryTableScan` is absent. The labels claim a plan shape, so the
benchmark should fail rather than mislabel itself if that shape ever changes. I
verified the assertion holds for all five of the benchmark's queries, including
the count-only and full-width ones where the scan output differs.
I could not re-run the benchmark itself in my current environment —
`exec:java` cannot bind a socket there, unrelated to this change — so the
numbers in the description are the previous run's. Only labels and comments
moved, so they still describe the same measurement, but flagging it rather than
implying I re-measured.
--
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]
