sunchao commented on PR #5607:
URL:
https://github.com/apache/datafusion-comet/pull/5607#issuecomment-5536037477
Reviewed head **`b517646`** against base `90c1dd9` with five independent
scopes. **The change adds little code, but the default execution choice is not
justified by the supplied performance results. I also found a new build
failure.**
1. **[P1] The benchmark breaks Spark 3.4/3.5 test compilation.**
[`describe`](https://github.com/apache/datafusion-comet/blob/b517646ff95d1e9a2b086a1802fddbdd3b805d87/spark/src/test/scala/org/apache/spark/sql/benchmark/CometBinaryLengthBenchmark.scala#L206)
returns `String`, but those Spark versions’ `withSQLConf` returns `Unit`.
Current CI reports exactly `found: Unit, required: String`. I independently
reproduced this with an extracted compilation. Capturing the result inside the
configuration block and returning it afterward compiles with both API
signatures. [CI
failure](https://github.com/apache/datafusion-comet/actions/runs/33795817298/job/100916316855)
2. **[P2] The new default route is slower in every reported benchmark
shape.**
The author’s `length` results show **164 → 206 ms at 64 B (+25.6%)** and
**1193 → 1522 ms at 1 KB (+27.6%)** versus the previous Comet/Spark fallback.
These are author-reported measurements; I did not independently reproduce the
timings. They argue for retaining fallback until a faster implementation or
representative pipeline measurements justify changing it. [Reported
results](https://github.com/apache/datafusion-comet/pull/5607)
The dispatcher adds a JVM callback and Arrow bridging per
expression/batch. Its [binary
getter](https://github.com/apache/datafusion-comet/blob/b517646ff95d1e9a2b086a1802fddbdd3b805d87/spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegenInput.scala#L281)
also allocates and copies the payload merely to read its length. **The old
fallback copies binary values too**, so copying alone does not establish the
cause of the measured regression.
3. **The benchmark should capture the actual timed execution plan.**
[`describe`](https://github.com/apache/datafusion-comet/blob/b517646ff95d1e9a2b086a1802fddbdd3b805d87/spark/src/test/scala/org/apache/spark/sql/benchmark/CometBinaryLengthBenchmark.scala#L207)
invokes `df.noop()` and then prints the original SELECT’s plan. I verified
that Spark 4.1.3’s `noop()` executes a separate write command with its own
`QueryExecution`. Capture that command through a query listener and assert the
intended operators. Also measure native downstream consumers and a projection
computing all three lengths from the same input. Those cases would test the
claimed benefit of preserving the Comet pipeline.
**For design and complexity, I favor a small native binary-length
implementation.** Reusing `CodegenDispatchFallback` is consistent with existing
code and introduces no unnecessary framework. The execution machinery is
nevertheless expensive for an operation that only needs adjacent Arrow offsets.
Arrow 58.4 already provides binary length kernels. A narrow adapter could
share byte-length handling between `length` and `octet_length`, preserve
Spark’s wrapping multiplication for `bit_length`, and avoid payload
materialization and JVM bridging. Three local component tests passed for nulls,
empty/arbitrary bytes, sliced arrays, and wide values. This validates the
underlying kernels, not a completed Comet implementation.
The performance and plan-reporting concerns were already raised in the
discussion; the compilation failure is the new finding. At the last CI check:
**4 failed, 13 passed, 6 running, 6 skipped**. No full Comet build or
end-to-end benchmark was run locally.
--
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]
