snmvaughan opened a new pull request, #7577: URL: https://github.com/apache/datafusion-comet/pull/7577
## Which issue does this PR close? Part of #7241. ## Rationale for this change Comet picks some behaviors by the running Spark version, because the Spark fix leaves nothing in the plan for Comet to detect. The checks for fixes that shipped in patch releases were spread over four places, each with its own version table: `SparkFixes`, `ArraySetSupport.normalizesFloats`, `RegrSparkVersions` and `CometPercentile.keepsSignedZeroKeys`. A fork that backports one of these fixes ahead of a release, so that its Spark reports an older version, had to find and change each of them. The checks also read the version in two ways. `SparkFixes`, `normalizesFloats` and `keepsSignedZeroKeys` use `Utils.majorMinorPatchVersion` and treat a version it can't parse as unfixed. `RegrSparkVersions` predates that helper and has its own parser: it accepts any string that starts with `x.y`, so `4.0.5_vendor` reads as `4.0.5`, and for anything else it falls back to `SPARK_VERSION >= "3.5"`. `CometAggregateSuite` pinned that fallback with `v3.5.9 -> true`. For such a string the regression checks and the others came to different conclusions. ## What changes are included in this PR? - `SparkFixes` holds every patch-release check, one predicate per fix, each with its own table of releases and the doc comment it had before. From `ArraySetSupport`: `arraySetFunctionsNormalizeFloats` (SPARK-54918, SPARK-59602), renamed because the object no longer says what it normalizes. From `RegrSparkVersions`: `slopeFiltersVarByPairNulls` (SPARK-48719) and `r2DegenerateCasesSwapped` (SPARK-55969). From `CometPercentile`: `keepsSignedZeroKeys` (SPARK-45599). The shared `fixedIn` for SPARK-58821 and SPARK-60118 is split, so each of those has its own table too. The tables are the ones the checks had, in the same order. - Every check reads the version with `Utils.majorMinorPatchVersion` and counts a version it can't parse as unfixed. `RegrSparkVersions`, with its regex and its `>= "3.5"` fallback, is gone. That is the one change in behavior, and it only shows for strings a real build does not report: `v3.5.9` and `4.0.5_vendor` were fixed for the two regression checks and are now unfixed, as they already were for the others, and a bare `4` now reads as `4.0.0`, which does not have SPARK-55969. Vendor suffixes like `-amzn-1` and extra numeric parts parse, so they are unaffected. - The `SparkFixes` header comment no longer says that other fixes keep their own checks, and says how a check is written. - `SparkFixesSuite` pins every table, so the boundaries are checked on every Spark version in one run. The regression and array set tables moved here from `CometAggregateSuite` and `CometArrayExpressionSuite` with their rows unchanged, apart from the two `v3.5.9` rows, which are now covered by a test that a version that does not parse counts as unfixed. The percentile check had no table, so it has a new one. - `adding_a_new_expression.md`, the `review-comet-pr` skill, `floating_point.md` and the array audit named the old locations and now name `SparkFixes`. Not in this PR: - `CometMode.keysNaNsTogether` (SPARK-45599 for `mode`) is still in the open #7115, so it can move once that merges. - `isSparkVersionAtLeast355` in the Spark 3.5 `ShimCometScanExec` stays where it is. It is a shim-level check, and it throws on a version that does not parse where the others treat it as unfixed. - The parser itself. #7404 makes `Utils.majorMinorPatchVersion` Comet's own and also edits `RegrSparkVersions.majorMinorPatch`, which this PR deletes, so whichever merges second has a one-hunk conflict there. ## How are these changes tested? - `SparkFixesSuite` (6 tests, all pass locally): the tables for every predicate, a vendor suffix that parses, and the strings that do not. - The tests that used the predicates in `CometAggregateSuite` (`regr_r2` over a reversed input) and `CometArrayExpressionSuite` (`array set signed-zero support levels` and the array set answers) call them through `SparkFixes` now. They compile, but I did not run them: they need a SparkContext. - The predicates were also compared with the ones they replace over 2,390 version strings: majors 2 to 6, minors 0 to 8 and patches 0 to 12, each plain and with `-SNAPSHOT`, `-amzn-1` and a `.7.3.2.30000-15` build number, plus the forms without a patch. They return the same answers everywhere except for the strings the parser rejects and a bare `4` for `regr_r2`, which are the change described above. This changes how every patch-release check reads the Spark version, and the default profile does not reach the older patch lines. Could a maintainer apply `run-all-spark-profiles`? I can't add labels from a fork. -- 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]
