andygrove commented on PR #5192: URL: https://github.com/apache/datafusion-comet/pull/5192#issuecomment-5179609112
I built this branch locally and ran benchmarks so we have numbers on the record for the outer variants. There was no synthetic-data benchmark for `GenerateExec`, only the TPC-DS micro query in `CometTPCDSMicroBenchmark`, which needs generated TPC-DS data. So I added an `explodeExecBenchmark` to `CometExecBenchmark` and ran that. I am happy to push it as a separate PR, or you are welcome to fold it into this one. ## Setup Apple M3 Max, JDK 17.0.10, Spark 4.1.3 / Scala 2.13 (the repo default), release native build with `RUSTFLAGS="-Ctarget-cpu=native"`, `local[5]`, 10M input rows written to snappy Parquet. Two data shapes: - **mixed**: one row in five holds a NULL array, one in five holds an empty array, the rest hold 1 to 3 elements. This is the shape the outer variants exist to handle and the one that drives `ListEmptyToNullExpr` down its slow path. - **all non-empty**: every array holds 1 to 3 elements, so the outer cases take the `ListEmptyToNullExpr` fast path. The benchmark checks `findFirstNonCometOperator` on the executed plan before timing, so none of these numbers are accidentally measuring a silent fallback. All six cases planned fully natively. Two independent runs, best time in ms: | query | shape | Spark | Comet | Comet relative | | --- | --- | --- | --- | --- | | `explode` | mixed | 164 / 152 | 146 / 145 | 1.1X / 1.0X | | `explode_outer` | mixed | 168 / 169 | 122 / 122 | **1.4X / 1.4X** | | `posexplode_outer` | mixed | 165 / 161 | 171 / 169 | **1.0X / 1.0X** | | `explode` | all non-empty | 182 / 171 | 91 / 95 | 2.0X / 1.8X | | `explode_outer` | all non-empty | 166 / 158 | 89 / 95 | 1.9X / 1.7X | | `posexplode_outer` | all non-empty | 177 / 169 | 129 / 133 | 1.4X / 1.3X | ## What I take from this **`explode_outer` is a clear win.** 1.4X on the mixed shape and 1.7X to 1.9X when the arrays are dense. That was the open question on this PR and I think it is answered. **`ListEmptyToNullExpr` is not costing anything measurable.** On the dense shape, plain `explode` with no wrapper and `explode_outer` with the wrapper come out the same, 91 and 95 against 89 and 95. So the extra per-batch scan is effectively free even when it finds nothing to do. On the mixed shape `explode_outer` is actually faster than plain `explode`, 122 against 145. I believe that is DataFusion's `preserve_nulls = false` filtering cost rather than anything this PR introduced, since it shows up on the non-outer path. **`posexplode_outer` on the mixed shape shows no speedup.** Comet's best time came out marginally behind Spark in both runs, 171 and 169 against 165 and 161. Spark's stdev is 16 to 18ms there so the gap itself sits inside the noise, but the direction was consistent across both runs and there is clearly no gain. **The `pos` branch looks like where the time goes.** Comparing within the same run on the same data, `explode_outer` at 122ms against `posexplode_outer` at 169ms, and 89 to 95ms against 129 to 133ms on the dense shape. That is roughly plus 40ms either way, about 40% on top. `ListPositionsExpr::evaluate` fills a `Vec<i32>` with a scalar `push` per element in a nested loop, which is a fair amount of per-element work for what is just a repeated ramp. That is pre-existing rather than something you added here, and it also affects plain `posexplode`. I do not think any of this blocks the PR. `explode_outer` is the headline feature and it delivers. But it seemed worth knowing that `posexplode_outer` is at parity on the shape that matters, and that the positions builder rather than the empty-to-null pass is the thing to look at if we want to improve it. Would you be up for filing a follow-up issue for `ListPositionsExpr` so it does not get lost? ## Caveats on reading the table Cases land in the 90 to 180ms range and Spark's stdev ranges from 3 to 25ms, so anything under about 10ms of difference is noise. The two shapes use different Parquet files, so cross-shape comparisons carry some scan-cost difference as well. The within-run, same-file comparisons are the trustworthy ones, which is why I leaned on the `explode_outer` against `posexplode_outer` delta rather than on absolute numbers. -- 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]
