snmvaughan opened a new issue, #7241: URL: https://github.com/apache/datafusion-comet/issues/7241
### What is the problem the feature request solves? 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 are spread over several places, each with its own version table: - `SparkFixes` (#7171): SPARK-58821 and SPARK-60118 - `ArraySetSupport.normalizesFloats`: SPARK-54918 and SPARK-59602 - `RegrSparkVersions` (#6042): SPARK-48719 and SPARK-55969 - `CometMode.keysNaNsTogether` (#7115): SPARK-45599 A fork that backports one of these fixes ahead of a release, so that its Spark reports an older version, has to find and change each of them. This came up in the #7171 review. The checks also read the version in two ways. `SparkFixes`, `normalizesFloats` and `keysNaNsTogether` parse it with Spark's `VersionUtils.majorMinorPatchVersion`, through `Utils.majorMinorPatchVersion`, and treat a version they can't parse as unfixed. `RegrSparkVersions` came before that helper, so it has its own parser: it accepts any string that starts with `x.y` and ignores the rest, and for anything else falls back to the minor-version rule, `SPARK_VERSION >= "3.5"`. `CometAggregateSuite` pins that fallback with `"v3.5.9" -> true`. For a version string such as `v3.5.9` or `4.0.5_vendor`, the regression checks and the others come to different conclusions. ### Describe the potential solution Once #7171 is in, move the other checks into `SparkFixes` with one version parser, and their version tables into `SparkFixesSuite`, as the #7171 review suggested. That needs one rule for the strings the two parsers read differently: either keep `RegrSparkVersions`' rule for its two checks, or move them to `VersionUtils` and update the `CometAggregateSuite` rows that pin the fallback. ### Additional context #6776 asks for each version check to live in one place in Scala, and #6893 audited the behavior flags against that rule. Its remaining items are comments and the CollectSet check, not these predicates. Checks for feature releases, such as `isSpark35Plus`, are out of scope. ### Willingness to contribute I can contribute a fix for this bug independently -- 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]
