andygrove commented on issue #5363:
URL:
https://github.com/apache/datafusion-comet/issues/5363#issuecomment-5341453380
Ran a probe on the current worktree to put actual numbers on the review.
Apple M3 Max, JDK 17, release build, 1M rows,
`parquet.enable.dictionary=false`, `spark.master=local[1]`,
`spark.comet.exec.columnarToRow.native.enabled=false` (default). Deterministic
input: `SELECT REPEAT(CAST(id AS STRING), 10) AS c1, id AS c_long FROM
range(1M)`.
For each expression the probe times four query shapes per engine:
| # | Query | What's timed |
|---|---|---|
| 1 | `SELECT <base_col> FROM t` | scan + c2r baseline |
| 2 | `SELECT <expr> FROM t` | **current methodology** — scan + expr + c2r |
| 3 | `SELECT sum(xxhash64(<base_col>)) FROM t` | scan + hash + agg baseline
(no c2r) |
| 4 | `SELECT sum(xxhash64(<expr>)) FROM t` | scan + expr + hash + agg |
Row 4 minus row 3 approximates the expression alone (hash-agg absorbs the
result server-side, so nothing crosses JNI/row). Compare against row 2 to see
what the current benchmark is really measuring.
### Best-time results (ms per 1M rows)
**upper(c1)** — string in, string out (worst case: full-width string c2r)
| Case | Spark | Comet |
|---|---|---|
| 1 scan+c2r | 78 | 47 |
| 2 scan+EXPR+c2r *(current methodology)* | **245** | **237** |
| 3 scan+hash+agg | 67 | 52 |
| 4 scan+EXPR+hash+agg | **234** | **253** |
| isolated expr ≈ (4)–(3) | ~167 | ~201 |
Reported (row 2) says Spark/Comet ≈ 1.03× — essentially a tie. Isolated (row
4 − row 3) says Spark's `upper` is actually **~20% faster** than Comet's here.
The current methodology is hiding a regression.
**length(c1)** — string in, int out (cheap c2r)
| Case | Spark | Comet |
|---|---|---|
| 1 scan+c2r | 67 | 43 |
| 2 scan+EXPR+c2r *(current)* | 124 | **35** |
| 3 scan+hash+agg | 62 | 49 |
| 4 scan+EXPR+hash+agg | 172 | 42 |
| isolated expr ≈ (4)–(3) | ~110 | ~0 |
Reported ratio 3.5×. In reality Spark spends ~110 ms in the expression while
Comet's `length` on a scanned string is essentially free — the isolated ratio
is well over 100×. The methodology **understates** Comet's advantage. (Note
that Comet row 2 = 35 ms is even *lower* than the row-1 baseline of 43 ms, i.e.
adding `length` made the timed region go faster — the int output is cheaper to
c2r than the string input, so the "baseline" already includes cost that the
"with expression" version avoids. That should be impossible for a real per-row
expression benchmark; it's a giveaway that the timer is dominated by c2r, not
by the expression.)
**lpad(c1, 150, 'x')** — string in, much wider string out
| Case | Spark | Comet |
|---|---|---|
| 1 scan+c2r | 66 | 43 |
| 2 scan+EXPR+c2r *(current)* | **751** | **50** |
| 3 scan+hash+agg | 59 | 48 |
| 4 scan+EXPR+hash+agg | 700 | 71 |
| isolated expr ≈ (4)–(3) | ~641 | ~23 |
Reported ratio 15×; isolated ratio ~28×. Comet row 2 is ~54% c2r-overhead —
the reported ratio understates Comet by ~2×. This is the "output much wider
than input" class and it hits every string-widening case in
`CometStringExpressionBenchmark`.
**CAST(c_long AS STRING)** — narrow in, string out
| Case | Spark | Comet |
|---|---|---|
| 1 scan+c2r | 37 | 29 |
| 2 scan+EXPR+c2r *(current)* | 84 | **43** |
| 3 scan+hash+agg | 30 | 20 |
| 4 scan+EXPR+hash+agg | 83 | 33 |
| isolated expr ≈ (4)–(3) | ~53 | ~13 |
Reported ratio 1.95×; isolated ratio ~4.08×. About **70% of Comet's timed
region is scan+c2r rather than the cast**. Every row in
`CometCastNumericToStringBenchmark` and `CometCastTemporalToStringBenchmark`
has this shape.
### Plans (confirm the mechanism)
Current methodology, Comet side, has a JNI-crossing `CometColumnarToRow` on
top:
```
*(1) CometColumnarToRow
+- CometProject [upper(c1)#6], [upper(c1#3) AS upper(c1)#6]
+- CometNativeScan parquet [c1#3] ...
```
Aggregate sink stays fully native — nothing crosses:
```
CometHashAggregate [sum#14L], [Final], [sum(xxhash64(upper(c1#3), 42))]
+- CometExchange SinglePartition, ENSURE_REQUIREMENTS, CometNativeShuffle,
[plan_id=94]
+- CometHashAggregate [c1#3], [Partial],
[partial_sum(xxhash64(upper(c1#3), 42))]
+- CometNativeScan parquet [c1#3] ...
```
### Takeaways
- The reported "Comet vs Spark" ratio is a mix of scan ratio, c2r ratio, and
expression ratio, in proportions that depend on **the output type of the
expression** — so cross-expression comparisons within a single results file are
also distorted.
- Direction of the error is not consistent. For `upper` the reported number
**hides a Comet regression**. For `length`/`lpad`/`cast` it **understates**
Comet's advantage by 2×–100×. Anyone comparing numbers across PRs is comparing
distorted-in-different-ways figures.
- The aggregate-sink shape (row 4 in the probe) is what the reimplementation
should use for expressions whose output is a string or a complex type. Cost is
one extra hash per row, identical in both arms, and c2r drops to zero.
Environment: OpenJDK 17.0.10, macOS 26.5.1, Apple M3 Max; probe was a
`ScratchOverheadProbe`/`ScratchOverheadProbeSuite` in
`spark/src/test/scala/org/apache/spark/sql/benchmark/` (not committed).
--
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]
