[SPARK-58588][SQL] Return the broadcast hash join build side from JoinSelectionHelper instead of a Boolean - #57790
Open
LuciferYang wants to merge 2 commits into
Open
Conversation
…nSelectionHelper instead of a Boolean ### What changes were proposed in this pull request? `canPlanAsBroadcastHashJoin` already computed an `Option[BuildSide]` internally and then discarded the direction with `.isDefined`. A caller that needs the direction therefore had to reimplement the same composition: `PushDownJoinThroughUnion` (SPARK-58449) did exactly that, so one planner decision was modelled twice, with each model incomplete and the coupling invisible in the code. This PR lifts the composition into `JoinSelectionHelper.getBroadcastHashJoinBuildSide`, which returns `Option[BuildSide]`, and redefines `canPlanAsBroadcastHashJoin` as `.isDefined` on it. The truth table of `canPlanAsBroadcastHashJoin` is unchanged, including its two deliberate over-approximations of `JoinSelection`: join keys no hash join supports still reach the size branch, and the method does not model `SHUFFLE_MERGE` or `SHUFFLE_REPLICATE_NL` hints. Both are now stated in the scaladoc instead of being implicit. `PushDownJoinThroughUnion` then drops its own `canPlanAsBroadcastHashJoin` conjunct, since `None` from the new method already covers the equi-key and hash-joinable-key requirements, and shares a `branchJoin` helper between the rewrite and the guard's probe plan. The probe now rewrites the join condition to the branch output, which the previous guard did not need to do because it never extracted equi-join keys. ### Why are the changes needed? Two partial models of the same planner decision drift apart. Strengthening one of them, for example teaching `canPlanAsBroadcastHashJoin` about a hint it currently ignores, silently leaves the other stale, and nothing in the code says the two have to agree. ### Does this PR introduce _any_ user-facing change? No. `canPlanAsBroadcastHashJoin` keeps its truth table, and `spark.sql.optimizer.pushDownJoinThroughUnion.enabled` remains false by default. ### How was this patch tested? `JoinSuite.assertJoin` now also asserts that `getBroadcastHashJoinBuildSide` agrees with the build side the planner picked, which covers every case in that suite that reaches a broadcast hash join. `JoinSelectionHelperSuite` gains five cases asserting the direction itself: a hint on either or both sides, the fall back to the smaller side, a shuffle hash hint, no equi-join keys, and a null-aware anti join. The direction had no coverage before this PR, so both were checked by mutation: inverting `getSmallerSide` fails four cases, and returning `BuildLeft` for the null-aware anti join fails exactly the new case for it. Existing suites pass: `JoinSelectionHelperSuite` (19), `PushDownJoinThroughUnionSuite` in catalyst (24) and core (9), and `JoinSuite` (58). ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code
uros-b
approved these changes
Aug 5, 2026
Contributor
Author
cloud-fan
approved these changes
Aug 6, 2026
cloud-fan
left a comment
cloud-fan
left a comment
Contributor
There was a problem hiding this comment.
0 blocking, 1 non-blocking, 0 nits.
The implementation is coherent, but one new contract claim overstates what the deliberately approximate helper guarantees.
Suggestions (1)
- sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/PushDownJoinThroughUnion.scala:112: The new comment says
Nonesubsumes hash-key eligibility even though the helper deliberately preserves an unsupported-key over-approximation. -- see inline
Verification
I traced hint precedence and the size fallback from getBroadcastHashJoinBuildSide into SparkStrategies.JoinSelection, then checked the per-branch condition remapping and right-build gate in PushDownJoinThroughUnion. The paths agree for supported broadcast hash joins; for non-binary-stable equi keys, the helper intentionally returns a prospective size-selected side while the physical planner rejects broadcast hash join creation.
| * right side as a plain probe input, which is not reused, so the right side is read once per such | ||
| * branch instead of once in total. | ||
| * | ||
| * `getBroadcastHashJoinBuildSide` returning `None` also covers the joins where no broadcast hash |
Contributor
There was a problem hiding this comment.
None does not cover the hash-joinable-key requirement. With non-binary-stable equi keys, the helper can still return a size-selected BuildRight, while JoinSelection rejects the broadcast hash join on hashJoinSupport. Since preserving that over-approximation is intentional here, please remove this claim so callers do not treat the result as an actual planner choice.
…e guard's scaladoc `getBroadcastHashJoinBuildSide` does not carry the hash-joinable-key requirement: with equi keys no hash join supports, `noShufflePlannedBefore` becomes true and the size branch still answers `Some(...)`, while `JoinSelection` falls through to a sort merge join. Only the equi-key requirement of the removed `canPlanAsBroadcastHashJoin` conjunct is carried over. Tighten the method's own summary for the same reason: it returns `None` when a broadcast hash join is ruled out by the join shape or by a hint, which is narrower than "`JoinSelection` could not plan one".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
canPlanAsBroadcastHashJoinalready computed anOption[BuildSide]internally and then discarded the direction with.isDefined. A caller that needs the direction therefore had to reimplement the same composition:PushDownJoinThroughUnion(SPARK-58449) did exactly that, so one planner decision was modelled twice, with each model incomplete and the coupling invisible in the code.This PR lifts the composition into
JoinSelectionHelper.getBroadcastHashJoinBuildSide, which returnsOption[BuildSide], and redefinescanPlanAsBroadcastHashJoinas.isDefinedon it. The truth table ofcanPlanAsBroadcastHashJoinis unchanged, including its two deliberate over-approximations ofJoinSelection: join keys no hash join supports still reach the size branch, and the method does not modelSHUFFLE_MERGEorSHUFFLE_REPLICATE_NLhints. Both are now stated in the scaladoc instead of being implicit.PushDownJoinThroughUnionthen drops its owncanPlanAsBroadcastHashJoinconjunct, sinceNonefrom the new method already carries the equi-key requirement, and shares abranchJoinhelper between the rewrite and the guard's probe plan. The probe now rewrites the join condition to the branch output, which the previous guard did not need to do because it never extracted equi-join keys.Making the helper fully faithful to
JoinSelectionwould mean returningNoneunder aSHUFFLE_MERGEorSHUFFLE_REPLICATE_NLhint. That flipscanPlanAsBroadcastHashJoinfrom true to false for a hinted join whose size still qualifies, which changes whatPushDownLeftSemiAntiJoinpushes down, so it is a behavior change rather than a refactor and is left for a separate ticket.Why are the changes needed?
Two partial models of the same planner decision drift apart. Strengthening one of them, for example teaching
canPlanAsBroadcastHashJoinabout a hint it currently ignores, silently leaves the other stale, and nothing in the code says the two have to agree.Does this PR introduce any user-facing change?
No.
canPlanAsBroadcastHashJoinkeeps its truth table, andspark.sql.optimizer.pushDownJoinThroughUnion.enabledremains false by default.How was this patch tested?
JoinSuite.assertJoinnow also asserts thatgetBroadcastHashJoinBuildSideagrees with the build side the planner picked, which covers every case in that suite that reaches a broadcast hash join.JoinSelectionHelperSuitegains five cases asserting the direction itself: a hint on either or both sides, the fall back to the smaller side, a shuffle hash hint, no equi-join keys, and a null-aware anti join.The direction had no coverage before this PR, so both were checked by mutation: inverting
getSmallerSidefails four cases, and returningBuildLeftfor the null-aware anti join fails exactly the new case for it.Existing suites pass:
JoinSelectionHelperSuite(19),PushDownJoinThroughUnionSuitein catalyst (24) and core (9), andJoinSuite(58).Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code