chore: drop the redundant width_bucket shim registrations and guard serde uniqueness

[dwsmith1983]

Which issue does this PR close?

Closes #4485.

Rationale for this change

width_bucket has gone through a registered serde since #4538, but the Spark 3.5 and 4.x expression shims still returned the same WidthBucket -> CometWidthBucket entry from their version-specific math map. It merged over the same key with the same value, so it changed nothing, but nothing would have said so if the two had ever drifted apart. The combined serde map is built by merging sixteen groups in order, so a class registered twice silently takes whichever serde merges last.

What changes are included in this PR?

• The two stale shim entries are removed; both version-specific math maps are empty now.
• The shared math, map, string and misc maps are hoisted out of their merge expressions into base*Expressions values, with the version shim still merged last, and the combined map is assembled from a named group list instead of a chain of ++. Same entries, same order.
• A SerdeRegistrationSuite asserts that every version shim registers only classes the shared map does not, that no class is registered in more than one group, and that every group entry reaches the combined map by identity with the sizes adding up. It is registered in both PR build workflows.

How are these changes tested?

The first test failed before the dedupe with exactly math: WidthBucket. Planting WidthBucket in the hash group fails the second test with WidthBucket: math, hash and the third on the size equality, so both guards are load-bearing; the plant is not part of the change. The width_bucket SQL fixture still runs natively on Spark 3.5 and 4.0, the suite passes on both, the 3.4 and 4.1 profiles compile, and spotless is clean.

[sunchao]

Reviewed 8311fcca against base 3810936b. No verified P1/P2 findings.

Correctness

The shared math registry already maps WidthBucket to the same CometWidthBucket singleton that the Spark 3.5 and 4.x shims redundantly supplied. Removing those shim entries preserves the effective handler; Spark 3.4 already uses an empty math shim, and Spark 4.0/4.1 inherit the shared 4.x registration. The 272 shared entries and the order of all 16 merged groups are unchanged.

CometWidthBucket still delegates to Spark's code-generation dispatcher, with the same expression enablement and fallback checks. The available Spark 3.5/4.0 implementations agree on null propagation, invalid bounds and bucket counts, ascending/descending boundary handling, and interval inputs. This PR does not change that evaluation path.

The new suite checks shared/shim key overlap before that merge, checks group ownership before constructing the final map, and checks both serde identity and total size afterward. I compiled and ran the exact suite against the extracted registry with synthetic expression/serde types on Scala 2.12.17, 2.13.16 and 2.13.17: all three tests passed for each selected shim configuration. Restoring the shim duplicate failed the overlap test; inserting the same handler into another group failed both ownership and size checks. These focused checks validate registration and Scala typing, not full Spark/native integration. The repository's workflow-suite check also passed.

At 2026-09-12 20:28:20 UTC, the head has only a successful label check; CI and the other three pull-request workflows report action_required. The author's SQL-test claims are not independently verified by a completed product CI run. The latest synthetic merge uses a newer base; all six authored files match the reviewed head, but no product execution is attributed to that merge. Maintained Spark 3.4/4.1 sources were unavailable for direct comparison.

Performance

The combined map remains an immutable singleton with the same lookup path for each expression. The named group list and its traversal add bounded initialization work; the duplicate scans run only in tests. Hoisting the four base maps retains their map roots for the singleton's lifetime, with immutable structure sharing. There is no new per-row work or repeated registry construction per query.

Removing the redundant shim math maps also removes their unnecessary entry allocation and overwrite. This is a maintenance change, and no measured query speedup is claimed. I found no performance change requiring a benchmark or blocking revision.

Design

Naming the groups makes registry composition inspectable while preserving existing merge precedence. Separating each shared map from its shim contribution is necessary for detecting the stale-registration problem: checking only the completed map would lose that evidence. The cross-group test likewise inspects individual groups, so even a duplicate that points to the same serde is detected.

The suite's three checks cover distinct failure modes: shim shadowing, cross-group duplication, and a lost or replaced entry in the combined registry. Both Linux and macOS workflows include it, and Linux selects the supported Spark profiles. Keeping this invariant in tests avoids adding a new production initialization failure mode.

Abstraction & complexity

The change adds four named base maps and one fixed sequence of existing groups. It keeps the existing map types, version-shim interface, serde implementations and dispatch API. The explicit existential type annotations remain in place, and the focused compilation exercised both Scala collection generations.

This is a small, direct representation of the registry rather than a new registration framework. The comments explain where version-specific classes belong and why overlap is disallowed. No additional abstraction or blocking simplification is needed.

[andygrove]

The premise holds up. WidthBucket is in Spark 3.4.3's catalyst and width_bucket is in its FunctionRegistry, so the shared mathExpressions entry has been the live registration on 3.4 ever since #4538, and dropping the 3.5 and 4.x shim entries is a no-op. I also pulled every classOf[...] per group and got 272 entries with 272 distinct classes and no cross-group duplicates, both at your base and at current main, so the new guard will not fire on a rebase.

What bothers me is that we have no CI evidence for the one profile the argument rests on. width_bucket.sql carries -- MinSparkVersion: 3.5, which was added back when the expression really was wired only through the shims, and CometWidthBucketSuite only exists under the spark-3.5 and spark-4.x test roots. So Spark 3.4 runs nothing for width_bucket at all. Could we drop the MinSparkVersion line as part of this PR? The codegen dispatcher is not version gated and CometCodegenSuite already runs on 3.4, so I would expect it to pass, and then the 3.4 job actually exercises the path this change is about.

The other thing is that this closes #4485, but docs/source/contributor-guide/expression-audits/math_funcs.md still says width_bucket is wired via per-version CometExprShim rather than a CometExpressionSerde and links that issue. After this PR that sentence is false and the link points at a closed issue. The same note says interval input types are not exercised by Comet tests, and width_bucket.sql only has a single double column, so that part is still true. Since closing the issue retires that item, could we fix the line here and either add interval cases or file a follow-up to link from it?

[andygrove]

Correction on my MinSparkVersion point above. That line is still on this branch but it is already gone from main, removed by #3910. So Spark 3.4 does run width_bucket.sql today and a rebase picks that up for free. Ignore that paragraph. CometWidthBucketSuite is still only under the spark-3.5 and spark-4.x test roots, but the SQL fixture is the coverage that matters here, so there is nothing for you to do.

The math_funcs.md point stands.

[rich7420]

@dwsmith1983 thanks for the patch

[dwsmith1983]

could we fix the line here and either add interval cases or file a follow-up to link from it?

Fixed the audit note: width_bucket is a CometCodegenDispatch registered once in the shared math group, on every Spark line. Added day-time and year-month interval cases to width_bucket.sql, built with make_dt_interval and make_ym_interval over the existing table plus literal forms. The MinSparkVersion line is still on main as of today, so I dropped it here; the fixture passes on 3.4 and 3.5 locally.

[sunchao]

Re-reviewed 46c6a97c against 1d0ce5fe, following the previous review. No new P1/P2 findings.

The new authored commit addresses the audit-note and interval-coverage requests: the note now describes the shared CometCodegenDispatch registration, the SQL fixture drops its Spark 3.5 minimum, and it adds day-time/year-month interval queries using columns and literals. The fixture runner disables constant folding and checks Comet execution plus Spark answer agreement. The column cases include nulls and values below, within, and above the bounds.

The registry, shims, dispatcher, and uniqueness suite are byte-identical to the prior reviewed head. The remaining increment matches the inherited base update. Current source checks confirm 272 unique shared registrations, the same 16-group order, and unchanged duplicate detection before the shim/group merges. The workflow-suite check passes. These changes add test coverage without changing runtime dispatch or per-row work.

Current CI, CodeQL, and Delta workflows require approval and each has zero jobs. The author's Spark 3.4/3.5 local passes remain author-reported. This follow-up used source checks, with direct Spark semantics comparison on maintained 3.5/4.0 only; maintained 3.4/4.1/4.2 sources were unavailable. No current-head Spark/JNI execution or speedup is claimed.

[sunchao]

Checked e1468922 against a8e8157e. No new or remaining P1/P2 findings. The existing approval is preserved.

The new commit only merges the current base. The authored contribution is unchanged, as are all 16 reviewed implementation, shim, dispatcher, documentation and fixture files. The earlier audit-note and interval-coverage requests remain addressed. Fresh source checks confirm 272 unique shared registrations, the same 16-group order and unchanged duplicate guards. The workflow-suite check passes for both Linux and macOS.

Current CI and CodeQL require approval and have zero jobs. Only the label check succeeded. This follow-up used source equivalence and maintained Spark 3.5/4.0 checks, with no new Spark/JNI execution or benchmark. Maintained 3.4/4.1/4.2 sources remain unavailable.

[sunchao]

Re-reviewed 92b48992 against 3868760f. No new or remaining P1/P2 findings. The existing approval is preserved.

This update only merges the newer base. The authored contribution is unchanged, and all 16 reviewed implementation, shim, dispatcher, documentation and fixture files are byte-identical to e1468922. Fresh checks still find 272 unique shared registrations, the same 16-group order, and the three unchanged uniqueness guards. The audit-note and interval-coverage requests remain addressed. The workflow-suite check passes for Linux and macOS.

The inherited CI changes now run the default Spark profile on PRs and move non-default profiles to nightly/opt-in coverage. The suite remains registered. Current CI and CodeQL require approval and have zero jobs. The only green check is labeling, whose log shows the base checkout. This follow-up used source checks and maintained Spark 3.5/4.0 semantics, with no fresh Spark/JNI execution or benchmark. Maintained 3.4/4.1/4.2 sources remain unavailable.

[sunchao]

Re-reviewed 78a15da9 against 8c229a70. No new or remaining P1/P2 findings. The existing approval is preserved.

This update only merges the newer base. The authored contribution is unchanged. Fifteen reviewed registry, shim, dispatcher, fixture and documentation files are byte-identical to 92b48992. The inherited datetime.scala changes affect day-of-week handlers and leave the interval constructor dispatch used by this fixture unchanged. Fresh checks confirm 272 unique shared registrations, the same 16-group order and all three uniqueness guards. The audit-note and interval-coverage requests remain addressed, and the workflow-suite check passes for Linux and macOS.

Current CI and CodeQL require approval and have zero jobs. Only the label check passed. This follow-up used source checks and maintained Spark 3.5/4.0 semantics, with no fresh Spark/JNI execution or benchmark. Maintained 3.4/4.1/4.2 sources remain unavailable.

[sunchao]

Re-reviewed eb36b20a against 36146a87. No new or remaining P1/P2 findings. The existing approval is preserved.

This update only merges the newer base. The authored contribution is unchanged. Fifteen relevant files remain byte-identical, including the registry, dispatcher, uniqueness suite and interval fixture. The inherited 4.1+ shim change handles TIME extraction and does not alter WidthBucket routing. Fresh checks confirm 272 unique shared registrations, the same 16-group order and all three uniqueness guards. The audit-note and interval-coverage requests remain addressed. The workflow-suite check passes for Linux and macOS.

Current CI and CodeQL require approval and have zero jobs. Only the label check passed. This follow-up used source checks and maintained Spark 3.5/4.0 semantics, with no fresh Spark/JNI execution or benchmark. Maintained 3.4/4.1/4.2 sources remain unavailable.