andygrove opened a new pull request, #6563: URL: https://github.com/apache/datafusion-comet/pull/6563
## Which issue does this PR close? This is the follow-up I promised in my #5750 review, so there's no separate issue. #5750 fixed #5701 by limiting native execution for float elements to Spark 4.2.0. **Stacked on #5750, which is in the merge queue.** The first commit is #5750's change. Review the second one, 757522e3e4. I'll rebase onto `main` and mark this ready once #5750 lands. cc @peterxcli, since this builds on your #5750. ## Rationale for this change After #5750, `array_distinct` and `array_union` fall back to Spark for float or double elements on every Spark version except 4.2.0. 4.2.0 normalizes the arguments of these functions in the plan (SPARK-54918). SPARK-59602 replaced that rewrite on `branch-4.0`, `branch-4.1` and `branch-4.2`: from 4.0.5, 4.1.4 and 4.2.1, Spark normalizes inside the expressions instead. Comet then receives the raw input, and DataFusion folds `-0.0` into `0.0` only in a flat array and compares NaNs by their bits. Those releases are still release candidates (`v4.0.5-rc1`, `v4.1.4-rc2`, `v4.2.1-rc1`). Once they ship, though, every Spark 4 user who upgrades would get the projection fallback for these functions. On #5750 I measured that fallback at about 15x slower than native for an `array<double>` workload. ## What changes are included in this PR? - **Native:** `SparkArraySetOp` (`spark_array_distinct` and `spark_array_union`) runs `normalize_nested_floats` on each argument and then delegates to DataFusion, the same pattern as `SparkArrayExtrema`. It is registered next to `SparkArrayRemove`. - **Serde:** element types with a float at any depth go to the `spark_` variants, the way `CometArrayRemove` picks `spark_array_remove`. Other element types still go straight to DataFusion. - **Gate:** `ArraySetSupport.normalizesFloats` admits 4.0.5+, 4.1.4+ and 4.2+. 3.4, 3.5, 4.0.0 to 4.0.4 and 4.1.0 to 4.1.3 still fall back, because they keep `-0.0` and `0.0` apart in a flat array, and normalizing can't reproduce that. - **Opt-in:** older Spark merges NaNs and nested zeros too, so the documented opt-in difference narrows to signed zeros. - **Fixtures:** in the 4.0/4.1 signed-zero fixture, the `array_distinct` and `array_union` cases are now `spark_answer_only`. Which path runs there depends on the patch release: fallback on 4.0.4 and 4.1.3, native on 4.0.5 and 4.1.4. `expect_fallback` would therefore break when the pom moves to those versions. `CometArrayExpressionSuite` checks the routing instead. The 4.2+ fixture now checks the default path rather than the opt-ins, and `SqlFileTestParserSuite` follows both changes. - **Docs:** `floating-point.md`, `expressions.md` and the array audit. By default nothing changes on any Spark release that has shipped. 4.2.0 already ran natively, and there the extra normalization repeats what the plan already did. ## How are these changes tested? - **Rust:** four unit tests on `SparkArraySetOp` cover NaN payloads and nulls in a flat array, union order across both sides, nested lists and structs, and a scalar argument. All four fail when the normalization is removed. `cargo fmt` and clippy (`--all-targets -D warnings`) pass. - **Opt-in arms:** the NaN test gains nested cases, and the nested signed-zero test gains values. Both now have an opt-in arm that runs on every version. That arm lets CI see the bug, because 4.2.0 normalizes in the plan and the SPARK-59602 releases aren't in the matrix yet. Every Spark version merges these elements, and the nested cases put the positive zero first, so older Spark's first-equal-element result matches too. - **Spark 4.1:** `CometArrayExpressionSuite` and `SqlFileTestParserSuite` passed 85 tests. `CometSqlFileTestSuite`, `CometCodegenFuzzSuite` and `CometFloatSemanticsSuite` passed 1047. - **Spark 4.2:** `CometArrayExpressionSuite`, `SqlFileTestParserSuite` and the `expressions/array/` fixtures passed 161 tests, including the 4.2+ fixture on the default path. - **Without the routing:** with `ArraySetSupport.function` returning the plain DataFusion names on Spark 4.1, the opt-in arms of the NaN and nested tests fail, and so does the routing check. I'll add `run-all-spark-profiles` after the rebase, since the arms differ by profile. -- 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]
