peterxcli commented on PR #5166: URL: https://github.com/apache/datafusion-comet/pull/5166#issuecomment-5444221531
Thanks for the detailed look. Point-by-point: **Array JVM dispatch**: fair ask on visibility — I've added a `getCompatibleNotes()` entry (which feeds the generated Compatibility Guide) documenting that array inputs containing floats route through the JVM codegen dispatcher on 4.2+, while scalar/struct inputs stay native, and that `spark.comet.exec.scalaUDF.codegen.enabled=false` falls those cases back to Spark. One framing correction: pre-PR the array path wasn't "fully native and fast" in any useful sense — it was fully native and *wrong* (#4966), since `spark.comet.exec.strictFloatingPoint` defaults to `false`. Dispatch is the fastest correct option today. Benchmarking the dispatch cost is tracked in #5312, which also covers whether to extend normalization to 3.4–4.1. **Spark internal**: expanded the doc comment on `CometExecUtils.normalizeFloatingNumbers` to note it's `private[sql]`, exists in all supported versions (3.4–4.2), and that any upstream rename fails the profile build loudly — it's a direct compile-time reference (not reflection) compiled against every Spark profile in CI, so the "loud test" is the build itself. Added guidance to shim per-version if it ever changes shape. **SQL gating**: the mechanism is the `-- MinSparkVersion: 4.2` / `-- MaxSparkVersion: 4.1` headers at the top of each file — they're parsed directives (`SqlFileTestParser`), documented in the [SQL file tests contributor guide](https://datafusion.apache.org/comet/contributor-guide/sql-file-tests.html), not a directory convention. **Idempotency**: Spark 4.2's normalization (SPARK-57298, apache/spark@49908a2cf92) lives inside `CollectSet.convertToBufferElement`/`eval`, not in the analyzer/optimizer — so the plan Comet serializes never arrives pre-wrapped and double-normalization cannot occur; `needNormalize` short-circuiting on `KnownFloatingPointNormalized` covers the hypothetical anyway. I've added the SPARK-57298 reference to the code comment. Given the wrap can't happen by construction, I'd skip the serialized-plan assertion test. -- 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]
