You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/08/05 09:54:10 UTC
[PR] [SPARK-58588][SQL] Return the broadcast hash join build side from JoinSelectionHelper instead of a Boolean [spark]
LuciferYang opened a new pull request, #57790:
URL: https://github.com/apache/spark/pull/57790
### 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.
Making the helper fully faithful to `JoinSelection` would mean returning `None` under a `SHUFFLE_MERGE` or `SHUFFLE_REPLICATE_NL` hint. That flips `canPlanAsBroadcastHashJoin` from true to false for a hinted join whose size still qualifies, which changes what `PushDownLeftSemiAntiJoin` pushes 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 `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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58588][SQL] Return the broadcast hash join build side from JoinSelectionHelper instead of a Boolean [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #57790:
URL: https://github.com/apache/spark/pull/57790#issuecomment-5193469287
cc @cloud-fan @dongjoon-hyun
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58588][SQL] Return the broadcast hash join build side from JoinSelectionHelper instead of a Boolean [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #57790:
URL: https://github.com/apache/spark/pull/57790#issuecomment-5235036154
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/213315948949987ef48dac6c8b8fa467a016afb2
- merged into branch-4.x https://github.com/apache/spark/commit/1b1d9c72610c8ae9ae09b468e4a7eed8c30c60b1
- merged into branch-4.3 https://github.com/apache/spark/commit/5ab28627719fa1234cebe5df295c02b50851d3d1
*Posted by `merge_spark_pr.py`*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58588][SQL] Return the broadcast hash join build side from JoinSelectionHelper instead of a Boolean [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #57790:
URL: https://github.com/apache/spark/pull/57790#discussion_r3727130838
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/PushDownJoinThroughUnion.scala:
##########
@@ -79,53 +75,67 @@ case class PushDownJoinThroughUnion(override val conf: SQLConf)
val deduped = dedupRight(right)
(deduped, AttributeMap(right.output.zip(deduped.output)))
}
- val leftRewrites = AttributeMap(unionHeadOutput.zip(child.output))
- val newCond = joinCond.map(_.transform {
- case a: Attribute if leftRewrites.contains(a) => leftRewrites(a)
- case a: Attribute if rightRewrites.contains(a) => rightRewrites(a)
- })
- Join(child, newRight, joinType, newCond, hint)
+ branchJoin(join, unionHeadOutput, child, newRight, rightRewrites)
}
u.withNewChildren(newChildren)
}
}
+ /**
+ * The join for one `Union` branch: the branch on the left, `newRight` on the right, and the
+ * condition rewritten from the `Union` output to the outputs of both new children.
+ */
+ private def branchJoin(
+ join: Join,
+ unionHeadOutput: Seq[Attribute],
+ child: LogicalPlan,
+ newRight: LogicalPlan,
+ rightRewrites: AttributeMap[Attribute]): Join = {
+ val leftRewrites = AttributeMap(unionHeadOutput.zip(child.output))
+ val newCond = join.condition.map(_.transform {
+ case a: Attribute if leftRewrites.contains(a) => leftRewrites(a)
+ case a: Attribute if rightRewrites.contains(a) => rightRewrites(a)
+ })
+ Join(child, newRight, join.joinType, newCond, join.hint)
+ }
+
/**
* Whether every join produced by the rewrite is expected to broadcast its right side.
*
- * `canPlanAsBroadcastHashJoin` is not enough. It holds when either side is broadcastable, and
- * for an inner join the planner may build from either side, choosing the smaller one when both
- * qualify. The rewrite replaces the `Union` on the left with one of its children, so the build
- * side is decided per branch against a smaller left. Any branch that ends up building from the
- * left leaves its copy of the 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.
+ * Asking whether a broadcast hash join is possible is not enough: for an inner join the planner
+ * may build from either side, choosing the smaller one when both qualify. The rewrite replaces
+ * the `Union` on the left with one of its children, so the build side is decided per branch
+ * against a smaller left. Any branch that ends up building from the left leaves its copy of the
+ * 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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58588][SQL] Return the broadcast hash join build side from JoinSelectionHelper instead of a Boolean [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #57790: [SPARK-58588][SQL] Return the broadcast hash join build side from JoinSelectionHelper instead of a Boolean
URL: https://github.com/apache/spark/pull/57790
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58588][SQL] Return the broadcast hash join build side from JoinSelectionHelper instead of a Boolean [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #57790:
URL: https://github.com/apache/spark/pull/57790#discussion_r3727249079
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/PushDownJoinThroughUnion.scala:
##########
@@ -79,53 +75,67 @@ case class PushDownJoinThroughUnion(override val conf: SQLConf)
val deduped = dedupRight(right)
(deduped, AttributeMap(right.output.zip(deduped.output)))
}
- val leftRewrites = AttributeMap(unionHeadOutput.zip(child.output))
- val newCond = joinCond.map(_.transform {
- case a: Attribute if leftRewrites.contains(a) => leftRewrites(a)
- case a: Attribute if rightRewrites.contains(a) => rightRewrites(a)
- })
- Join(child, newRight, joinType, newCond, hint)
+ branchJoin(join, unionHeadOutput, child, newRight, rightRewrites)
}
u.withNewChildren(newChildren)
}
}
+ /**
+ * The join for one `Union` branch: the branch on the left, `newRight` on the right, and the
+ * condition rewritten from the `Union` output to the outputs of both new children.
+ */
+ private def branchJoin(
+ join: Join,
+ unionHeadOutput: Seq[Attribute],
+ child: LogicalPlan,
+ newRight: LogicalPlan,
+ rightRewrites: AttributeMap[Attribute]): Join = {
+ val leftRewrites = AttributeMap(unionHeadOutput.zip(child.output))
+ val newCond = join.condition.map(_.transform {
+ case a: Attribute if leftRewrites.contains(a) => leftRewrites(a)
+ case a: Attribute if rightRewrites.contains(a) => rightRewrites(a)
+ })
+ Join(child, newRight, join.joinType, newCond, join.hint)
+ }
+
/**
* Whether every join produced by the rewrite is expected to broadcast its right side.
*
- * `canPlanAsBroadcastHashJoin` is not enough. It holds when either side is broadcastable, and
- * for an inner join the planner may build from either side, choosing the smaller one when both
- * qualify. The rewrite replaces the `Union` on the left with one of its children, so the build
- * side is decided per branch against a smaller left. Any branch that ends up building from the
- * left leaves its copy of the 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.
+ * Asking whether a broadcast hash join is possible is not enough: for an inner join the planner
+ * may build from either side, choosing the smaller one when both qualify. The rewrite replaces
+ * the `Union` on the left with one of its children, so the build side is decided per branch
+ * against a smaller left. Any branch that ends up building from the left leaves its copy of the
+ * 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
Review Comment:
[529d10a](https://github.com/apache/spark/pull/57790/commits/529d10a273b775a2e915b1c196f7ca5f329ce029) address this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58588][SQL] Return the broadcast hash join build side from JoinSelectionHelper instead of a Boolean [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #57790:
URL: https://github.com/apache/spark/pull/57790#issuecomment-5235038811
Thank you @cloud-fan @dongjoon-hyun @uros-b
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org