You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "MrHappyEnding (via GitHub)" <gi...@apache.org> on 2026/08/05 10:03:51 UTC
[PR] [SPARK-58384][SQL] Preserve null semantics in OptimizeJoinCondition [spark]
MrHappyEnding opened a new pull request, #57791:
URL: https://github.com/apache/spark/pull/57791
### What changes were proposed in this pull request?
Restrict `OptimizeJoinCondition` to rewriting null-safe equality patterns only at the root of a join condition or beneath `AND`/`OR`.
The rule no longer performs this rewrite beneath `NOT`, where the difference between `NULL` and `FALSE` is observable.
### Why are the changes needed?
When exactly one operand is `NULL`, the original pattern returns `NULL`, while `<=>` returns `FALSE`.
This difference does not matter at the root of a join condition, but under `NOT` it can cause Spark to incorrectly keep extra rows.
### Does this PR introduce *any* user-facing change?
Yes. Join conditions containing this pattern beneath `NOT` now return the correct rows.
For the SPARK-58384 reproduction, the result changes from four rows to only:
```text
[0,10,1,22]
```
### How was this patch tested?
Added tests covering:
- No rewrite beneath `NOT`.
- Continued rewriting beneath `AND` and `OR`.
- The end-to-end join result with null values.
Ran:
```text
build/sbt 'catalyst/testOnly org.apache.spark.sql.catalyst.optimizer.OptimizeJoinConditionSuite'
build/sbt 'sql/testOnly org.apache.spark.sql.DataFrameJoinSuite -- -z "SPARK-58384"'
build/sbt 'catalyst/scalastyle' 'catalyst/Test/scalastyle' 'sql/Test/scalastyle'
```
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Codex (GPT-5)
--
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-58384][SQL] Preserve null semantics in OptimizeJoinCondition [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #57791: [SPARK-58384][SQL] Preserve null semantics in OptimizeJoinCondition
URL: https://github.com/apache/spark/pull/57791
--
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-58384][SQL] Preserve null semantics in OptimizeJoinCondition [spark]
Posted by "MrHappyEnding (via GitHub)" <gi...@apache.org>.
MrHappyEnding commented on code in PR #57791:
URL: https://github.com/apache/spark/pull/57791#discussion_r3733643369
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJoinCondition.scala:
##########
@@ -30,16 +30,24 @@ object OptimizeJoinCondition extends Rule[LogicalPlan] with PredicateHelper {
override def apply(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsPattern(JOIN), ruleId) {
case j @ Join(_, _, _, condition, _) if condition.nonEmpty =>
- val newCondition = condition.map(_.transformWithPruning(_.containsPattern(OR), ruleId) {
- case Or(EqualTo(l, r), And(IsNull(c1), IsNull(c2)))
- if (l.semanticEquals(c1) && r.semanticEquals(c2))
- || (l.semanticEquals(c2) && r.semanticEquals(c1)) =>
- EqualNullSafe(l, r)
- case Or(And(IsNull(c1), IsNull(c2)), EqualTo(l, r))
- if (l.semanticEquals(c1) && r.semanticEquals(c2))
- || (l.semanticEquals(c2) && r.semanticEquals(c1)) =>
- EqualNullSafe(l, r)
- })
+ val newCondition = condition.map(optimizeCondition)
j.copy(condition = newCondition)
}
+
+ // Rewriting the pattern to EqualNullSafe maps NULL to FALSE, so only recurse through And/Or.
+ private def optimizeCondition(condition: Expression): Expression = condition match {
+ case Or(EqualTo(l, r), And(IsNull(c1), IsNull(c2)))
+ if (l.semanticEquals(c1) && r.semanticEquals(c2))
+ || (l.semanticEquals(c2) && r.semanticEquals(c1)) =>
+ EqualNullSafe(l, r)
+ case Or(And(IsNull(c1), IsNull(c2)), EqualTo(l, r))
+ if (l.semanticEquals(c1) && r.semanticEquals(c2))
+ || (l.semanticEquals(c2) && r.semanticEquals(c1)) =>
+ EqualNullSafe(l, r)
+ case And(left, right) =>
+ And(optimizeCondition(left), optimizeCondition(right))
Review Comment:
Thanks! Updated the traversal to preserve tree-pattern pruning and reuse unchanged AND/OR nodes.
--
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-58384][SQL] Preserve null semantics in OptimizeJoinCondition [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #57791:
URL: https://github.com/apache/spark/pull/57791#issuecomment-5218245286
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/cea566b22b3de6879f3b38e6def2cef0d417a731
- merged into branch-4.x https://github.com/apache/spark/commit/5db725450a90057709ad0db261cdfc50f9c42850
- merged into branch-4.3 https://github.com/apache/spark/commit/f67d154fb1792996cf88cdf4b85b4c78542ca17c
*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-58384][SQL] Preserve null semantics in OptimizeJoinCondition [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #57791:
URL: https://github.com/apache/spark/pull/57791#issuecomment-5190799303
Thank you @MrHappyEnding! cc @cloud-fan who merged the original `OptimizeJoinCondition` rule
--
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-58384][SQL] Preserve null semantics in OptimizeJoinCondition [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #57791:
URL: https://github.com/apache/spark/pull/57791#discussion_r3727695387
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeJoinCondition.scala:
##########
@@ -30,16 +30,24 @@ object OptimizeJoinCondition extends Rule[LogicalPlan] with PredicateHelper {
override def apply(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsPattern(JOIN), ruleId) {
case j @ Join(_, _, _, condition, _) if condition.nonEmpty =>
- val newCondition = condition.map(_.transformWithPruning(_.containsPattern(OR), ruleId) {
- case Or(EqualTo(l, r), And(IsNull(c1), IsNull(c2)))
- if (l.semanticEquals(c1) && r.semanticEquals(c2))
- || (l.semanticEquals(c2) && r.semanticEquals(c1)) =>
- EqualNullSafe(l, r)
- case Or(And(IsNull(c1), IsNull(c2)), EqualTo(l, r))
- if (l.semanticEquals(c1) && r.semanticEquals(c2))
- || (l.semanticEquals(c2) && r.semanticEquals(c1)) =>
- EqualNullSafe(l, r)
- })
+ val newCondition = condition.map(optimizeCondition)
j.copy(condition = newCondition)
}
+
+ // Rewriting the pattern to EqualNullSafe maps NULL to FALSE, so only recurse through And/Or.
+ private def optimizeCondition(condition: Expression): Expression = condition match {
+ case Or(EqualTo(l, r), And(IsNull(c1), IsNull(c2)))
+ if (l.semanticEquals(c1) && r.semanticEquals(c2))
+ || (l.semanticEquals(c2) && r.semanticEquals(c1)) =>
+ EqualNullSafe(l, r)
+ case Or(And(IsNull(c1), IsNull(c2)), EqualTo(l, r))
+ if (l.semanticEquals(c1) && r.semanticEquals(c2))
+ || (l.semanticEquals(c2) && r.semanticEquals(c1)) =>
+ EqualNullSafe(l, r)
+ case And(left, right) =>
+ And(optimizeCondition(left), optimizeCondition(right))
Review Comment:
Please keep the OR tree-pattern guard and return the original AND/OR node when neither child changes. Without those checks, this rule now traverses and allocates a fresh Boolean tree for every unrelated join condition that the previous `transformWithPruning` skipped.
--
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