andygrove opened a new pull request, #6561: URL: https://github.com/apache/datafusion-comet/pull/6561
## Which issue does this PR close? Closes #5701 on `branch-1.1`, for 1.1.0-rc2. ## Rationale for this change This is the `branch-1.1` backport of #5750. #5262 upgraded DataFusion to 55, whose `array_distinct` and `array_union` treat `-0.0` and `0.0` as one value and return `0.0` for it. Comet runs both natively and reports them as compatible, so 1.1.0-rc1 differs from Spark on every release that keeps the zeros apart: all of 3.4 and 3.5, 4.0 before 4.0.5, and 4.1 before 4.1.4. With Spark 4.1.3, `array_distinct(array(0.0, -0.0, 1.0))` returns `[0.0, 1.0]`, where Spark and 1.0.0 return all three values. 1.0.0 ran on DataFusion 54.1, which kept them apart. #5750 has the details. `branch-1.0` doesn't need it for the regression, because #5262 isn't on it. ## What changes are included in this PR? A cherry-pick of #5750. When the element type holds a `FLOAT` or `DOUBLE` at any depth, `array_distinct` and `array_union` now fall back to Spark on every Spark version except 4.2.0, the only release that normalizes their arguments in the plan. Other element types stay native, and the `allowIncompatible` opt-ins still select the native path. The floating-point compatibility guide explains the boundary and the cost of the fallback. #5750 is in the merge queue, so I prepared this from its head (01d9a57c87). Once it merges, I'll re-pick its squash commit with `-x`, so that the message names the commit on `main`. Outside the files below, every hunk is the same as upstream's, including all of the production code. The adaptations: - `CometFloatSemanticsSuite` doesn't exist on this branch, so the hunk that drops its #5701 known gap is left out. - `floating-point.md`: this branch's page doesn't have the `array_min`/`array_max` paragraph or the `array_remove`/`sort_array` section that surround the new section on `main`. So only the new "Array distinct and union" section is added, at the end of the page. - `expressions.md`: kept this branch's `array_contains` row and took #5750's `array_distinct` row. - `CometArrayExpressionSuite`: the import block conflicted, because this branch doesn't import `ArrayMax` and `ArrayMin`. I kept this branch's imports and added #5750's `ArrayDistinct` and `ArrayUnion`. ## How are these changes tested? On this branch, with the default Spark 4.1 profile: - `CometArrayExpressionSuite` and `SqlFileTestParserSuite`: 83 passed, including the five new array set tests and the rewritten fixture check. - `CometSqlFileTestSuite` and `CometCodegenFuzzSuite`: 598 passed. The Spark 3 and Spark 4.2+ signed-zero fixtures are gated by version and don't run on 4.1. - Without the fix, the new fixture fails on this branch. With `arrays.scala`, `QueryPlanSerde.scala` and `Utils.scala` put back to their `branch-1.1` versions, along with `CometArrayExpressionSuite`, which refers to the new serde, `array_set_signed_zero_spark_4_0_4_1.sql` fails on its first query: a `CometProject` returns `[0.0, 1.0]` for `array_distinct(array(0.0, -0.0, 1.0))`, where Spark returns `[0.0, -0.0, 1.0]`. - Spotless, scalastyle and prettier pass. #5750 passed on all five Spark profiles at its head. I didn't run the other profiles locally. CI runs them on this branch, along with the Spark SQL suites, which #5750's run on `main` skipped. -- 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]
