chore: drop the redundant width_bucket shim registrations and guard serde uniqueness - #5873
chore: drop the redundant width_bucket shim registrations and guard serde uniqueness#5873dwsmith1983 wants to merge 1 commit into
Conversation
…erde uniqueness The Spark 3.5 and 4.x shims still registered WidthBucket over the same key the shared math map registers, a no-op that nothing would have caught had the two drifted. Remove both entries, hoist the shared maps out of their merge expressions, assemble the combined map from a named group list, and add a suite asserting that shims only add classes, that no class lives in two groups, and that every entry reaches the combined map unchanged. Closes apache#4485
sunchao
left a comment
There was a problem hiding this comment.
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.
Which issue does this PR close?
Closes #4485.
Rationale for this change
width_buckethas gone through a registered serde since #4538, but the Spark 3.5 and 4.x expression shims still returned the sameWidthBucket -> CometWidthBucketentry 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?
math,map,stringandmiscmaps are hoisted out of their merge expressions intobase*Expressionsvalues, 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.SerdeRegistrationSuiteasserts 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. PlantingWidthBucketin the hash group fails the second test withWidthBucket: math, hashand the third on the size equality, so both guards are load-bearing; the plant is not part of the change. Thewidth_bucketSQL 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.