andygrove commented on issue #7241: URL: https://github.com/apache/datafusion-comet/issues/7241#issuecomment-6446326293
Thanks for filing this, @snmvaughan. I checked the list against main and I support the change. I have a few corrections to the inventory and two design questions. **Inventory** - `CometPercentile.keepsSignedZeroKeys` in `aggregates.scala` belongs on the list. It is on main from #6928, it covers SPARK-45599 for percentile, and it uses `Utils.majorMinorPatchVersion` like the others. - `CometMode.keysNaNsTogether` is still only in open PR #7115, so that check can't move until the PR merges. Could it go in a follow-up, or would you rather wait? - `isSparkVersionAtLeast355` in the spark-3.5 `ShimCometScanExec` is a third way to read the version. It uses `VersionUtils`, but it throws on an unparseable version where the others treat it as unfixed. I'd leave it out of scope because it is a shim-level check. Could the issue say so, so nobody goes looking for it? **Parser rule** I'd use `VersionUtils` for all of them and drop the `RegrSparkVersions` regex and the `sparkVersion >= "3.5"` fallback. The strings the two parsers read differently, such as `v3.5.9` or `4.0.5_vendor`, are not strings a real build reports, because vendor suffixes like `-amzn-1` and extra numeric parts already parse. Keeping the old rule for two checks would leave the inconsistency this issue is meant to remove. The cost is that those two `CometAggregateSuite` rows change from `true` to `false`, which the PR should say. **Shape of the move** - Each fix should keep its own predicate and its own version table. `fixedIn` is shared today only because SPARK-58821 and SPARK-60118 shipped in the same releases, and the regr and array checks have different thresholds. With a table per fix, a fork that backports one fix edits one line and leaves the others alone. - `SparkFixes.scala` should hold the predicates and tables and little else, so a fork that patches it conflicts in one small file when it merges main. - I considered putting these behind shim methods. They would need a patch-level check inside each source set anyway, and each shim could only be tested on the Spark version it was built for, whereas `SparkFixesSuite` can pin every version in one run. So I'd keep the predicates in common code and use a shim only when a fix lines up with a minor-version line. **Things the PR needs to touch** - `adding_a_new_expression.md` names `RegrSparkVersions.r2DegenerateCasesSwapped` as the model, `review-comet-pr/SKILL.md` names `RegrSparkVersions.slopeFiltersVarByPairNulls`, and `floating_point.md` points at `ArraySetSupport`. All three should point at `SparkFixes`. - The `SparkFixes` header comment says other fixes keep their own checks, so it needs rewriting. - The version tables in `CometAggregateSuite` and `CometArrayExpressionSuite` move to `SparkFixesSuite`. `CometArrayExpressionSuite` and the audit docs call `ArraySetSupport.normalizesFloats` directly, so they need updating. Optional, as a separate change: for a version string `VersionUtils` can't parse, should the checks fail loudly instead of defaulting to unfixed? A string we can't read gives wrong results quietly today. That changes behavior for everyone, so I'd keep it out of this PR. -- 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]
