andygrove commented on PR #5166: URL: https://github.com/apache/datafusion-comet/pull/5166#issuecomment-5441544331
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. Reusing Spark's own `NormalizeFloatingNumbers.normalize` instead of reimplementing the recursion is the right instinct, and gating it to `Partial` and `Complete` so that merge paths are untouched looks correct to me. Splitting the SQL coverage into a version-specific normalization file and a fallback file is a good way to keep both behaviors pinned. A few things. **The array case now costs a JVM round trip on every batch** For an `ARRAY<DOUBLE>` input, `normalize` produces an `ArrayTransform`, which Comet runs through the codegen dispatcher. So on Spark 4.2, `collect_set` over a float-containing array goes from fully native to per-batch JVM dispatch. That is a correctness-driven trade and I am not arguing against it, but it should be visible to users rather than only in this PR description. Could `getIncompatibleReasons` or the compatibility docs say that array-typed `collect_set` on floating-point elements is no longer end-to-end native on 4.2+? A rough number would also help. How much slower is the dispatched path for a `collect_set(array<double>)` over a realistic batch? **Depending on a Spark internal** `NormalizeFloatingNumbers.normalize` is `private[sql]` with no stability guarantee, and `CometExecUtils` gets at it by living in `org.apache.spark.sql.comet`. That is a legitimate technique and Comet does it elsewhere, but it is worth a note in the doc comment saying which Spark versions the method exists in and what to do if it changes shape. Right now the comment explains what the wrapper does but not that it is a compatibility risk. Related: is there a test that would fail loudly if a future Spark renamed or moved it? A compile failure is fine as a signal, but only if some profile actually compiles this path. **How does the version-specific SQL coverage get gated?** `collect_set_normalization.sql` is described as 4.2+ and `collect_set_floating_fallback.sql` as pre-4.2. What mechanism keeps each from running on the wrong version? If it is a `-- Config:` header or a directory convention, saying so in a comment at the top of each file would help, otherwise the next person adding a version-specific fixture has to reverse-engineer it. **One check on idempotency** If Spark 4.2 performs this normalization itself somewhere in the analyzer or optimizer, Comet would be normalizing an already-normalized child. `needNormalize` short-circuits on `KnownFloatingPointNormalized`, so I believe it is a no-op, but it would be worth confirming with a test that asserts the serialized plan contains exactly one `NormalizeNaNAndZero` rather than two. Where in Spark 4.2 does the normalization actually live? A link in the comment would make the whole change much easier to verify. -- 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]
