Move the version checks for Spark patch-release fixes into SparkFixes #7241

Activity

1. github-actions

   added

   requires-triage

   on Oct 26, 2026
2. andygrove commented on Oct 27, 2026

   

   andygrove

   on Oct 27, 2026

   Member

   More actions

   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 fix: pick the signed zero Spark ranks first in native percentile #6928, it covers SPARK-45599 for percentile, and it uses Utils.majorMinorPatchVersion like the others.
   • CometMode.keysNaNsTogether is still only in open PR fix: fall back to Spark for mode on FLOAT and DOUBLE before Spark 3.5.2 #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.