andygrove opened a new pull request, #7115: URL: https://github.com/apache/datafusion-comet/pull/7115
## Which issue does this PR close? Closes #6738. Part of #6385. ## Rationale for this change Before SPARK-45599 (fixed in Spark 3.5.2 and 4.0.0), `mode` counts values in an `OpenHashMap` that hashes a float by its bits but matches keys with `==`. No two NaNs match, so every NaN row is a key of its own with a count of 1. Native `mode` canonicalizes NaN and counts them together. With `spark.comet.expression.Mode.allowIncompatible=true`, `mode` over `NaN, NaN, NaN, 1.0, 1.0` returns `NaN` on Spark 3.4.3 where Spark returns `1.0`. There is no tie, so the opt-in's documented reason (tie-breaking, #3970) does not cover it. ## What changes are included in this PR? - `CometMode.getSupportLevel` returns `Unsupported` for `FLOAT` and `DOUBLE` input on Spark 3.4, 3.5.0 and 3.5.1, with the reason in the fallback message and the generated compatibility guide. `Unsupported` rather than `Incompatible`, because `allowIncompatible` would otherwise run it and return a wrong answer with no tie. From 3.5.2, nothing changes. - The version check is `CometMode.keysNaNsTogether`, which reads the patch version as `ArraySetSupport.normalizesFloats` does. - The doc comment on `Mode` in `mode.rs` and the comment in `CometMode.convert` said that every version collapses NaN and that versions before 4.2 key by `Double.equals`. Both hold only from 3.5.2 and 4.0.0, and now say so. - `mode.sql` loses its `DOUBLE` sections, which move to `mode_floating_point.sql` (`MinSparkVersion: 3.5`) unchanged except for the corrected comments. New `mode_floating_point_spark_3_4.sql` (`MaxSparkVersion: 3.4`) holds the reproduction and the `DOUBLE` and `FLOAT` queries as `expect_fallback`. - The user guide row for `mode` and the contributor guide's `mode` bullet mention the fallback. An alternative is to count each NaN separately in the native accumulator, which would keep `mode` native on 3.4. It is opt-in and incompatible for ties already, and the epic keeps version differences in one Scala policy (#6385), so this takes the fallback. The issue allows either. ## How are these changes tested? - `mode_floating_point_spark_3_4.sql` fails on 3.4 without the new branch (checked by removing it), and passes with it. `mode_floating_point.sql` passes on 3.5, 4.1 and 4.2; `mode.sql` passes on 3.4, 3.5, 4.1 and 4.2. - `CometAggregateSuite` gets `mode float keys by Spark patch version` (3.4.3, 3.5.0, 3.5.1 and 3.5.1-SNAPSHOT on one side, 3.5.2 through 4.2 on the other) and `mode support level for float input follows the Spark version`, which pass on 3.4 and 4.1. - `MinSparkVersion` reads only the minor version, so `mode_floating_point.sql` also runs on a local 3.5.0 or 3.5.1, where it would fail. CI pins 3.5.8. -- 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]
