sam-1112 commented on PR #5802:
URL:
https://github.com/apache/datafusion-comet/pull/5802#issuecomment-5637176258
Thanks — this was a real methodology bug.
`DatasetToBenchmark.noop()` on Spark 4.x goes through
`df.write.format("noop").mode(Overwrite).save()`, which creates a separate
write `QueryExecution` from the logical plan. As a result, each timed sample
included write planning, while `assertRoute` was inspecting a different
physical plan from the one actually executed.
I fixed this in `6961541` using the first option you suggested: the
benchmark now prepares and validates the physical action that is actually
timed, rather than measuring an end-to-end noop write.
The updated lifecycle is:
* `prepareQuery` forces physical planning outside the timer and returns the
resulting SparkPlan.
* `assertRoute` inspects that same prepared plan.
* The timed path consumes the physical plan directly with `executeColumnar`
/ `execute` and `foreachPartition`; it does not create another `QueryExecution`
or go through `DataFrameWriter`.
* `SQLExecution.withNewExecutionId` is only used for execution metrics / job
grouping; the body still executes the supplied physical plan.
For shuffle samples, I also preserved fresh execution.
`CometShuffleExchangeExec` and Spark's `ShuffleExchangeExec` cache their
shuffle RDD/dependency, so re-executing the same exchange instance would reuse
shuffle output. Before `startTiming()`, the benchmark therefore copies each
shuffle exchange and rebuilds only the ancestors needed to wire in the fresh
exchange instance. Unaffected children, including the MapSort-containing
subtree, remain shared with the route-validated plan. This gives each measured
sample a fresh shuffle while keeping physical planning outside the timed region.
I avoided `transformUp` / `withNewChildren` here because an equal-but-new
exchange can be discarded through `fastEquals`; the benchmark rebuilds the
affected parents by child identity instead.
The first-action path was corrected in the same way: it plans and validates
the physical plan before timing, does not execute it during validation, resets
the dispatcher statistics immediately before the sample, and directly executes
the prepared plan.
I reran the paired measurements on the same machine. Please disregard the
earlier noop-write results.
Environment: Spark 4.1.3, Java 21, Apple M4, `local[1]`, native shuffle, AQE
disabled. Steady-state results are medians of Best Per Row across five fresh
JVMs with alternating `fallback-first` / `dispatcher-first` ordering.
The values below are ns/row (lower is faster). Speedup is `fallback /
dispatcher`, so values above `1.00x` favor the dispatcher.
### Projection — 1M rows
| Shape | Dispatcher | Fallback | Speedup |
| --------------------- | ---------: | -------: | ------: |
| array, map 4, key 2 | 168.2 ns | 129.7 ns | 0.77x |
| array, map 32, key 2 | 1305.9 ns | 734.5 ns | 0.56x |
| array, map 4, key 8 | 359.5 ns | 179.0 ns | 0.50x |
| struct, map 4, key 2 | 145.5 ns | 177.4 ns | 1.22x |
| struct, map 32, key 2 | 1052.2 ns | 1172.0 ns | 1.11x |
| struct, map 4, key 8 | 461.1 ns | 334.6 ns | 0.73x |
| strict-double, map 4 | 123.4 ns | 100.4 ns | 0.81x |
| strict-double, map 32 | 898.4 ns | 454.1 ns | 0.51x |
### Native shuffle — 250k rows
| Shape | Dispatcher | Fallback | Speedup |
| --------------------- | ---------: | --------: | ------: |
| array, map 4, key 2 | 704.0 ns | 615.1 ns | 0.87x |
| array, map 32, key 2 | 4187.7 ns | 2844.4 ns | 0.68x |
| array, map 4, key 8 | 1141.8 ns | 968.8 ns | 0.85x |
| struct, map 4, key 2 | 904.3 ns | 724.4 ns | 0.80x |
| struct, map 32, key 2 | 6021.2 ns | 3598.1 ns | 0.60x |
| struct, map 4, key 8 | 2168.4 ns | 1361.1 ns | 0.63x |
| strict-double, map 4 | 562.7 ns | 449.4 ns | 0.80x |
| strict-double, map 32 | 3336.0 ns | 1815.5 ns | 0.54x |
With the corrected timing boundary, Spark fallback is faster in 14 of the 16
steady-state pairs. The dispatcher is faster only on the two struct projection
cases (small 1.22x, large 1.11x). All eight native-shuffle cases favor
fallback. Larger maps and wider nested keys are where the dispatcher is
slowest, not fastest.
I also reran first-action with three fresh JVMs per case.
Dispatcher/fallback latency is about 1.10–1.17x (dispatcher slower). Each
dispatcher process observes `compile_count=1` and `cache_hit_count=3`; fallback
remains `0 / 0`.
The qualitative picture is unchanged from the earlier noop-write runs: the
dispatcher is usually slower, with a win on some struct projections. The
absolute ns/row figures did change and should not be compared to the old table.
--
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]
