Repository navigation
Move the version checks for Spark patch-release fixes into SparkFixes #7241
Description
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(fix: match SPARK-58821 and SPARK-60118 in sequence and string to decimal cast #7171): SPARK-58821 and SPARK-60118ArraySetSupport.normalizesFloats: SPARK-54918 and SPARK-59602RegrSparkVersions(fix: gate the regr_r2 degenerate-case swap on the Spark patch release #6042): SPARK-48719 and SPARK-55969CometMode.keysNaNsTogether(fix: fall back to Spark for mode on FLOAT and DOUBLE before Spark 3.5.2 #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
Activity
andygrove commentedon Oct 27, 2026
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.keepsSignedZeroKeysinaggregates.scalabelongs 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 usesUtils.majorMinorPatchVersionlike the others.CometMode.keysNaNsTogetheris 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?isSparkVersionAtLeast355in the spark-3.5ShimCometScanExecis a third way to read the version. It usesVersionUtils, 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
VersionUtilsfor all of them and drop theRegrSparkVersionsregex and thesparkVersion >= "3.5"fallback. The strings the two parsers read differently, such asv3.5.9or4.0.5_vendor, are not strings a real build reports, because vendor suffixes like-amzn-1and 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 twoCometAggregateSuiterows change fromtruetofalse, which the PR should say.Shape of the move
fixedInis 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.scalashould hold the predicates and tables and little else, so a fork that patches it conflicts in one small file when it merges main.SparkFixesSuitecan 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.mdnamesRegrSparkVersions.r2DegenerateCasesSwappedas the model,review-comet-pr/SKILL.mdnamesRegrSparkVersions.slopeFiltersVarByPairNulls, andfloating_point.mdpoints atArraySetSupport. All three should point atSparkFixes.SparkFixesheader comment says other fixes keep their own checks, so it needs rewriting.CometAggregateSuiteandCometArrayExpressionSuitemove toSparkFixesSuite.CometArrayExpressionSuiteand the audit docs callArraySetSupport.normalizesFloatsdirectly, so they need updating.Optional, as a separate change: for a version string
VersionUtilscan'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.