You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "LukaZdravic (via GitHub)" <gi...@apache.org> on 2026/09/29 12:10:42 UTC
[PR] [SPARK-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
LukaZdravic opened a new pull request, #59134:
URL: https://github.com/apache/spark/pull/59134
### What changes were proposed in this pull request?
SQL ASOF JOIN rebuilds each STRUCT operand of `MATCH_CONDITION` from its fields, so both sides line up by position. The rebuilt struct is never NULL. This PR keeps it NULL when the original struct is NULL, as `If(IsNull(original), NULL, rebuilt)`, at every nesting level. Structs that can never be NULL, like `(a, b)` tuples, are unchanged.
### Why are the changes needed?
A NULL operand must never match, but a NULL struct did:
```sql
SET spark.sql.join.asofJoin.enabled=true;
SELECT r.tag
FROM VALUES (CAST(NULL AS STRUCT<a: INT>), 'l') AS t(k, tag)
ASOF JOIN VALUES (CAST(NULL AS STRUCT<a: INT>), 'rnull'), (named_struct('a', 1), 'r1') AS r(k, tag)
MATCH_CONDITION (t.k >= r.k);
```
- Before: `rnull`. The NULL struct was rebuilt as `{a: NULL}`, which compares as equal to another rebuilt NULL struct.
- After: no row, the same as a plain `WHERE t.k >= r.k`.
Nested structs had a second problem: `{e: NULL}` and `{e: {a: NULL}}` were rebuilt as the same value, so `{e: {a: NULL}} > {e: NULL}` found no match.
### Does this PR introduce _any_ user-facing change?
No. SQL ASOF JOIN is not in a released version yet.
### How was this patch tested?
- `ResolveAsOfJoinSuite`: a nullable struct operand gets the NULL guard, and a non-nullable one does not.
- `AsOfJoinSortMergeSQLSuite`: flat and nested NULL structs, with ANSI on and off. It covers NULL on both sides, on the left only, on the right only, LEFT ASOF JOIN, and the nested `{e: {a: NULL}} > {e: NULL}` case.
Without the fix, all three tests fail. The existing ASOF suites and the `join-asof-*.sql` golden files pass, and the golden outputs do not change.
The forward form of the nested case (`<`) also needs SPARK-59829, because forward joins still read the distance on master.
So waiting for https://github.com/apache/spark/pull/59102 to merge before adding that one test also.
### 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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on PR #59134:
URL: https://github.com/apache/spark/pull/59134#issuecomment-5913361236
Follow up from https://github.com/apache/spark/pull/59016, added test that was missing
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "SavicStefan (via GitHub)" <gi...@apache.org>.
SavicStefan commented on code in PR #59134:
URL: https://github.com/apache/spark/pull/59134#discussion_r4165149985
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2890,8 +2892,10 @@ object AsOfJoin {
(leftOperand.dataType, rightOperand.dataType) match {
case (leftStruct: StructType, rightStruct: StructType)
if MatchConditionTypes.usesIdenticalStructSort(leftStruct, rightStruct) =>
- if (isSqlTupleStructOperand(leftOperand) || isSqlTupleStructOperand(rightOperand)) {
- val pairs = collectStructLeafPairs(leftOperand, rightOperand, leftStruct)
+ if ((isSqlTupleStructOperand(leftOperand) || isSqlTupleStructOperand(rightOperand)) &&
+ !leftOperand.nullable && !rightOperand.nullable) {
Review Comment:
is there a reason SortMergeAsOfJoinExec requires the same number of sort keys on both sides, because like If one operand is a SQL tuple and the other is a STRUCT column, could we just return separate field expressions for that tuple and one whole struct key for the column?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -3105,17 +3091,21 @@ object AsOfJoin {
wrapCompositeOrderExpression(leafDiffs)
}
+ /** `splitNullableStructs = false` keeps a struct pair whole when either side can be NULL. */
private def collectStructLeafPairs(
leftOperand: Expression,
rightOperand: Expression,
- structType: StructType): Seq[(Expression, Expression)] = {
+ structType: StructType,
+ splitNullableStructs: Boolean = true): Seq[(Expression, Expression)] = {
structFieldExprs(leftOperand, structType)
.zip(structFieldExprs(rightOperand, structType))
.flatMap {
case (left, right) =>
- if (MatchConditionTypes.usesStructDecomposition(left.dataType, right.dataType)) {
+ val canSplit = splitNullableStructs || (!left.nullable && !right.nullable)
+ if (canSplit &&
+ MatchConditionTypes.usesStructDecomposition(left.dataType, right.dataType)) {
Review Comment:
I think this is the pattern and it looks better
```suggestion
if (splitNullableStructs || (!left.nullable && !right.nullable)) &&
MatchConditionTypes.usesStructDecomposition(left.dataType, right.dataType) =>
```
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on code in PR #59134:
URL: https://github.com/apache/spark/pull/59134#discussion_r4167447699
##########
sql/core/src/test/scala/org/apache/spark/sql/AsOfJoinSortMergeSQLSuite.scala:
##########
@@ -440,6 +448,117 @@ class AsOfJoinSortMergeSQLSuite extends QueryTest
Row(Timestamp.valueOf("2026-06-29 10:03:00"), 1, "v1.1")))
}
+ test("NULL whole STRUCT column MATCH_CONDITION never matches") {
+ // A NULL struct must never match, the same as a NULL scalar operand.
+ for {
+ (nullStruct, one, five) <- Seq(
+ ("CAST(NULL AS STRUCT<a: INT>)", "named_struct('a', 1)", "named_struct('a', 5)"),
+ ("CAST(NULL AS STRUCT<a: INT, b: INT>)",
+ "named_struct('a', 1, 'b', 1)", "named_struct('a', 5, 'b', 5)"),
+ ("CAST(NULL AS STRUCT<e: STRUCT<a: INT>>)",
+ "named_struct('e', named_struct('a', 1))", "named_struct('e', named_struct('a', 5))"),
+ // Non-NULL values of this type are all equal.
+ ("CAST(NULL AS STRUCT<e: STRUCT<>>)",
+ "named_struct('e', named_struct())", "named_struct('e', named_struct())"))
+ ansiEnabled <- Seq(true, false)
+ } {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ def asOfTags(joinType: String, left: String, op: String, rights: String): DataFrame =
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES ($left, 'l') AS t(k, tag)
+ |$joinType ASOF JOIN VALUES $rights AS r(k, tag)
+ | MATCH_CONDITION (t.k $op r.k)
+ |""".stripMargin)
+ checkSortMergeAsOf(
+ asOfTags("", nullStruct, ">=", s"($nullStruct, 'rnull'), ($one, 'r1')"), Nil)
+ checkSortMergeAsOf(asOfTags("", nullStruct, "<=", s"($one, 'r1')"), Nil)
+ checkSortMergeAsOf(asOfTags("", five, ">=", s"($nullStruct, 'rnull')"), Nil)
+ // LEFT ASOF JOIN keeps the NULL left row with NULL right columns.
+ checkSortMergeAsOf(
+ asOfTags("LEFT", nullStruct, ">=", s"($nullStruct, 'rnull'), ($one, 'r1')"),
+ Row(null) :: Nil)
+ }
+ }
+ }
+
+ test("nested STRUCT column MATCH_CONDITION keeps a NULL inner struct apart from NULL fields") {
+ // {e: NULL} sorts before {e: {a: NULL}}, as in a plain comparison.
+ val eNull = "named_struct('e', CAST(NULL AS STRUCT<a: INT>))"
+ val aNull = "named_struct('e', named_struct('a', CAST(NULL AS INT)))"
+ for {
+ (left, op, right) <- Seq((aNull, ">", eNull), (eNull, "<", aNull))
+ ansiEnabled <- Seq(true, false)
+ } {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ checkSortMergeAsOf(
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES ($left, 'l') AS t(k, tag)
+ |ASOF JOIN VALUES ($right, 'r') AS r(k, tag)
+ | MATCH_CONDITION (t.k $op r.k)
+ |""".stripMargin),
+ Row("r") :: Nil)
+ }
+ }
+ }
+
+ test("tuple vs nullable STRUCT column MATCH_CONDITION: a NULL struct does not end the scan") {
+ // A NULL struct and {NULL, NULL} have the same fields, so a sort by fields ties them. Both
+ // input orders must pick r11: a NULL row after a match must not stop the backward scan.
+ val allNull = "(named_struct('a', CAST(NULL AS INT), 'b', CAST(NULL AS INT)), 'allnull')"
+ val nullStruct = "(CAST(NULL AS STRUCT<a: INT, b: INT>), 'rnull')"
+ val r11 = "(named_struct('a', 1, 'b', 1), 'r11')"
+ for {
+ rights <- Seq(s"$allNull, $nullStruct, $r11", s"$nullStruct, $allNull, $r11")
+ ansiEnabled <- Seq(true, false)
+ } {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ withOrderedRightView(rights, "s, tag") {
+ checkSortMergeAsOf(
+ sql(
+ """
+ |SELECT r.tag
+ |FROM VALUES (1, 2) AS t(a, b)
+ |ASOF JOIN r
+ | MATCH_CONDITION ((t.a, t.b) >= r.s)
+ |""".stripMargin),
+ Row("r11") :: Nil)
+ }
+ }
+ }
+ }
+
+ test("tuple with a nullable inner STRUCT MATCH_CONDITION picks the closest row") {
+ // NULL and {a: NULL} have the same field, but the comparison ranks NULL lower. Both input
+ // orders must pick the row equal to the left one, in both directions.
+ val aNull = "named_struct('a', CAST(NULL AS INT))"
+ val nullA = "CAST(NULL AS STRUCT<a: INT>)"
+ val rAnull = s"(1, $aNull, 'r_anull')"
+ val rNull = s"(1, $nullA, 'r_null')"
+ for {
+ rights <- Seq(s"$rAnull, $rNull", s"$rNull, $rAnull")
+ ansiEnabled <- Seq(true, false)
+ } {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ withOrderedRightView(rights, "x, s, tag") {
+ def asOfTag(left: String, op: String): DataFrame =
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES (1, $left) AS t(x, s)
+ |ASOF JOIN r
+ | MATCH_CONDITION ((t.x, t.s) $op (r.x, r.s))
+ |""".stripMargin)
+ checkSortMergeAsOf(asOfTag(aNull, ">="), Row("r_anull") :: Nil)
+ checkSortMergeAsOf(asOfTag(nullA, "<="), Row("r_null") :: Nil)
+ }
+ }
+ }
+ }
+
Review Comment:
Added the missing test, thanks!
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "SavicStefan (via GitHub)" <gi...@apache.org>.
SavicStefan commented on code in PR #59134:
URL: https://github.com/apache/spark/pull/59134#discussion_r4166831148
##########
sql/core/src/test/scala/org/apache/spark/sql/AsOfJoinSortMergeSQLSuite.scala:
##########
@@ -440,6 +448,117 @@ class AsOfJoinSortMergeSQLSuite extends QueryTest
Row(Timestamp.valueOf("2026-06-29 10:03:00"), 1, "v1.1")))
}
+ test("NULL whole STRUCT column MATCH_CONDITION never matches") {
Review Comment:
it makes sense to use gridTest here
##########
sql/core/src/test/scala/org/apache/spark/sql/AsOfJoinSortMergeSQLSuite.scala:
##########
@@ -440,6 +448,117 @@ class AsOfJoinSortMergeSQLSuite extends QueryTest
Row(Timestamp.valueOf("2026-06-29 10:03:00"), 1, "v1.1")))
}
+ test("NULL whole STRUCT column MATCH_CONDITION never matches") {
+ // A NULL struct must never match, the same as a NULL scalar operand.
+ for {
+ (nullStruct, one, five) <- Seq(
+ ("CAST(NULL AS STRUCT<a: INT>)", "named_struct('a', 1)", "named_struct('a', 5)"),
+ ("CAST(NULL AS STRUCT<a: INT, b: INT>)",
+ "named_struct('a', 1, 'b', 1)", "named_struct('a', 5, 'b', 5)"),
+ ("CAST(NULL AS STRUCT<e: STRUCT<a: INT>>)",
+ "named_struct('e', named_struct('a', 1))", "named_struct('e', named_struct('a', 5))"),
+ // Non-NULL values of this type are all equal.
+ ("CAST(NULL AS STRUCT<e: STRUCT<>>)",
+ "named_struct('e', named_struct())", "named_struct('e', named_struct())"))
+ ansiEnabled <- Seq(true, false)
+ } {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ def asOfTags(joinType: String, left: String, op: String, rights: String): DataFrame =
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES ($left, 'l') AS t(k, tag)
+ |$joinType ASOF JOIN VALUES $rights AS r(k, tag)
+ | MATCH_CONDITION (t.k $op r.k)
+ |""".stripMargin)
+ checkSortMergeAsOf(
+ asOfTags("", nullStruct, ">=", s"($nullStruct, 'rnull'), ($one, 'r1')"), Nil)
+ checkSortMergeAsOf(asOfTags("", nullStruct, "<=", s"($one, 'r1')"), Nil)
+ checkSortMergeAsOf(asOfTags("", five, ">=", s"($nullStruct, 'rnull')"), Nil)
+ // LEFT ASOF JOIN keeps the NULL left row with NULL right columns.
+ checkSortMergeAsOf(
+ asOfTags("LEFT", nullStruct, ">=", s"($nullStruct, 'rnull'), ($one, 'r1')"),
+ Row(null) :: Nil)
+ }
+ }
+ }
+
+ test("nested STRUCT column MATCH_CONDITION keeps a NULL inner struct apart from NULL fields") {
+ // {e: NULL} sorts before {e: {a: NULL}}, as in a plain comparison.
+ val eNull = "named_struct('e', CAST(NULL AS STRUCT<a: INT>))"
+ val aNull = "named_struct('e', named_struct('a', CAST(NULL AS INT)))"
+ for {
+ (left, op, right) <- Seq((aNull, ">", eNull), (eNull, "<", aNull))
+ ansiEnabled <- Seq(true, false)
+ } {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ checkSortMergeAsOf(
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES ($left, 'l') AS t(k, tag)
+ |ASOF JOIN VALUES ($right, 'r') AS r(k, tag)
+ | MATCH_CONDITION (t.k $op r.k)
+ |""".stripMargin),
+ Row("r") :: Nil)
+ }
+ }
+ }
+
+ test("tuple vs nullable STRUCT column MATCH_CONDITION: a NULL struct does not end the scan") {
+ // A NULL struct and {NULL, NULL} have the same fields, so a sort by fields ties them. Both
+ // input orders must pick r11: a NULL row after a match must not stop the backward scan.
+ val allNull = "(named_struct('a', CAST(NULL AS INT), 'b', CAST(NULL AS INT)), 'allnull')"
+ val nullStruct = "(CAST(NULL AS STRUCT<a: INT, b: INT>), 'rnull')"
+ val r11 = "(named_struct('a', 1, 'b', 1), 'r11')"
+ for {
+ rights <- Seq(s"$allNull, $nullStruct, $r11", s"$nullStruct, $allNull, $r11")
+ ansiEnabled <- Seq(true, false)
+ } {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ withOrderedRightView(rights, "s, tag") {
+ checkSortMergeAsOf(
+ sql(
+ """
+ |SELECT r.tag
+ |FROM VALUES (1, 2) AS t(a, b)
+ |ASOF JOIN r
+ | MATCH_CONDITION ((t.a, t.b) >= r.s)
+ |""".stripMargin),
+ Row("r11") :: Nil)
+ }
+ }
+ }
+ }
+
+ test("tuple with a nullable inner STRUCT MATCH_CONDITION picks the closest row") {
+ // NULL and {a: NULL} have the same field, but the comparison ranks NULL lower. Both input
+ // orders must pick the row equal to the left one, in both directions.
+ val aNull = "named_struct('a', CAST(NULL AS INT))"
+ val nullA = "CAST(NULL AS STRUCT<a: INT>)"
+ val rAnull = s"(1, $aNull, 'r_anull')"
+ val rNull = s"(1, $nullA, 'r_null')"
+ for {
+ rights <- Seq(s"$rAnull, $rNull", s"$rNull, $rAnull")
+ ansiEnabled <- Seq(true, false)
+ } {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ withOrderedRightView(rights, "x, s, tag") {
+ def asOfTag(left: String, op: String): DataFrame =
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES (1, $left) AS t(x, s)
+ |ASOF JOIN r
+ | MATCH_CONDITION ((t.x, t.s) $op (r.x, r.s))
+ |""".stripMargin)
+ checkSortMergeAsOf(asOfTag(aNull, ">="), Row("r_anull") :: Nil)
+ checkSortMergeAsOf(asOfTag(nullA, "<="), Row("r_null") :: Nil)
+ }
+ }
+ }
+ }
+
Review Comment:
do we need to add also a positive test where a non-NULL struct with all NULL fields is the selected match? For example, with left {a: NULL, b: NULL} and right candidates NULL and {a: NULL, b: NULL}, >= should select the latter.
The current test includes all NULL, but expects r11 so it doesn’t explicitly verify that all NULL itself can match.
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on code in PR #59134:
URL: https://github.com/apache/spark/pull/59134#discussion_r4182138058
##########
sql/core/src/test/scala/org/apache/spark/sql/AsOfJoinSortMergeSQLSuite.scala:
##########
@@ -440,6 +448,136 @@ class AsOfJoinSortMergeSQLSuite extends QueryTest
Row(Timestamp.valueOf("2026-06-29 10:03:00"), 1, "v1.1")))
}
+ gridTest("NULL whole STRUCT column MATCH_CONDITION never matches")(Seq(
+ ("CAST(NULL AS STRUCT<a: INT>)", "named_struct('a', 1)", "named_struct('a', 5)"),
+ ("CAST(NULL AS STRUCT<a: INT, b: INT>)",
+ "named_struct('a', 1, 'b', 1)", "named_struct('a', 5, 'b', 5)"),
+ ("CAST(NULL AS STRUCT<e: STRUCT<a: INT>>)",
+ "named_struct('e', named_struct('a', 1))", "named_struct('e', named_struct('a', 5))"),
+ // Non-NULL values of this type are all equal.
+ ("CAST(NULL AS STRUCT<e: STRUCT<>>)",
+ "named_struct('e', named_struct())", "named_struct('e', named_struct())"))) {
+ case (nullStruct, one, five) =>
+ // A NULL struct must never match, the same as a NULL scalar operand.
+ Seq(true, false).foreach { ansiEnabled =>
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ def asOfTags(joinType: String, left: String, op: String, rights: String): DataFrame =
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES ($left, 'l') AS t(k, tag)
+ |$joinType ASOF JOIN VALUES $rights AS r(k, tag)
+ | MATCH_CONDITION (t.k $op r.k)
+ |""".stripMargin)
+ checkSortMergeAsOf(
+ asOfTags("", nullStruct, ">=", s"($nullStruct, 'rnull'), ($one, 'r1')"), Nil)
+ checkSortMergeAsOf(asOfTags("", nullStruct, "<=", s"($one, 'r1')"), Nil)
+ checkSortMergeAsOf(asOfTags("", five, ">=", s"($nullStruct, 'rnull')"), Nil)
+ // LEFT ASOF JOIN keeps the NULL left row with NULL right columns.
+ checkSortMergeAsOf(
+ asOfTags("LEFT", nullStruct, ">=", s"($nullStruct, 'rnull'), ($one, 'r1')"),
+ Row(null) :: Nil)
+ }
+ }
+ }
+
+ gridTest("whole STRUCT column MATCH_CONDITION matches a struct whose fields are all NULL")(Seq(
+ ">=" -> Seq(Row("allnull")), "<=" -> Seq(Row("allnull")), ">" -> Nil, "<" -> Nil)) {
+ case (op, expected) =>
+ // Two structs of NULL fields are equal, and a NULL struct never matches.
+ val allNull = "named_struct('a', CAST(NULL AS INT), 'b', CAST(NULL AS INT))"
+ val nullStruct = "CAST(NULL AS STRUCT<a: INT, b: INT>)"
+ Seq(true, false).foreach { ansiEnabled =>
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ checkSortMergeAsOf(
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES ($allNull, 'l') AS t(k, tag)
+ |ASOF JOIN VALUES ($nullStruct, 'rnull'), ($allNull, 'allnull') AS r(k, tag)
+ | MATCH_CONDITION (t.k $op r.k)
+ |""".stripMargin),
+ expected)
+ }
+ }
+ }
+
+ test("nested STRUCT column MATCH_CONDITION keeps a NULL inner struct apart from NULL fields") {
+ // {e: NULL} sorts before {e: {a: NULL}}, as in a plain comparison.
+ val eNull = "named_struct('e', CAST(NULL AS STRUCT<a: INT>))"
+ val aNull = "named_struct('e', named_struct('a', CAST(NULL AS INT)))"
+ for {
+ (left, op, right) <- Seq((aNull, ">", eNull), (eNull, "<", aNull))
+ ansiEnabled <- Seq(true, false)
+ } {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ checkSortMergeAsOf(
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES ($left, 'l') AS t(k, tag)
+ |ASOF JOIN VALUES ($right, 'r') AS r(k, tag)
+ | MATCH_CONDITION (t.k $op r.k)
+ |""".stripMargin),
+ Row("r") :: Nil)
+ }
+ }
+ }
+
+ test("tuple vs nullable STRUCT column MATCH_CONDITION: a NULL struct does not end the scan") {
+ // A NULL struct and {NULL, NULL} have the same fields, so a sort by fields ties them. Both
Review Comment:
Changed in latest commit
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "SavicStefan (via GitHub)" <gi...@apache.org>.
SavicStefan commented on PR #59134:
URL: https://github.com/apache/spark/pull/59134#issuecomment-5934623813
This rebuild here is not needed, we can compare the original operands directly and we would avoid making this fix, because spark already compares struct by positions and handles whole and nested NULLs.
We still need to keep the fix for sorting, because NULL starts needs to come before a non-NULL structs whose fields are all NULL.
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on code in PR #59134:
URL: https://github.com/apache/spark/pull/59134#discussion_r4165700980
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2890,8 +2892,10 @@ object AsOfJoin {
(leftOperand.dataType, rightOperand.dataType) match {
case (leftStruct: StructType, rightStruct: StructType)
if MatchConditionTypes.usesIdenticalStructSort(leftStruct, rightStruct) =>
- if (isSqlTupleStructOperand(leftOperand) || isSqlTupleStructOperand(rightOperand)) {
- val pairs = collectStructLeafPairs(leftOperand, rightOperand, leftStruct)
+ if ((isSqlTupleStructOperand(leftOperand) || isSqlTupleStructOperand(rightOperand)) &&
+ !leftOperand.nullable && !rightOperand.nullable) {
Review Comment:
No the check for the same number of sort keys isn't absolutely needed, we could always just return seperate field expressions but we do not gain anything by doing it that way.
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on PR #59134:
URL: https://github.com/apache/spark/pull/59134#issuecomment-5934904120
> This rebuild here is not needed, we can compare the original operands directly and we would avoid making this fix, because Spark already compares struct by positions and handles whole and nested NULLs.
>
> We still need to keep the fix for sorting, because NULL starts needs to come before a non-NULL structs whose fields are all NULL.
Great catch! Will work on removing rebuild all together
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59134:
URL: https://github.com/apache/spark/pull/59134#discussion_r4173425569
##########
sql/core/src/test/scala/org/apache/spark/sql/AsOfJoinSortMergeSQLSuite.scala:
##########
@@ -440,6 +448,136 @@ class AsOfJoinSortMergeSQLSuite extends QueryTest
Row(Timestamp.valueOf("2026-06-29 10:03:00"), 1, "v1.1")))
}
+ gridTest("NULL whole STRUCT column MATCH_CONDITION never matches")(Seq(
+ ("CAST(NULL AS STRUCT<a: INT>)", "named_struct('a', 1)", "named_struct('a', 5)"),
+ ("CAST(NULL AS STRUCT<a: INT, b: INT>)",
+ "named_struct('a', 1, 'b', 1)", "named_struct('a', 5, 'b', 5)"),
+ ("CAST(NULL AS STRUCT<e: STRUCT<a: INT>>)",
+ "named_struct('e', named_struct('a', 1))", "named_struct('e', named_struct('a', 5))"),
+ // Non-NULL values of this type are all equal.
+ ("CAST(NULL AS STRUCT<e: STRUCT<>>)",
+ "named_struct('e', named_struct())", "named_struct('e', named_struct())"))) {
+ case (nullStruct, one, five) =>
+ // A NULL struct must never match, the same as a NULL scalar operand.
+ Seq(true, false).foreach { ansiEnabled =>
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ def asOfTags(joinType: String, left: String, op: String, rights: String): DataFrame =
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES ($left, 'l') AS t(k, tag)
+ |$joinType ASOF JOIN VALUES $rights AS r(k, tag)
+ | MATCH_CONDITION (t.k $op r.k)
+ |""".stripMargin)
+ checkSortMergeAsOf(
+ asOfTags("", nullStruct, ">=", s"($nullStruct, 'rnull'), ($one, 'r1')"), Nil)
+ checkSortMergeAsOf(asOfTags("", nullStruct, "<=", s"($one, 'r1')"), Nil)
+ checkSortMergeAsOf(asOfTags("", five, ">=", s"($nullStruct, 'rnull')"), Nil)
+ // LEFT ASOF JOIN keeps the NULL left row with NULL right columns.
+ checkSortMergeAsOf(
+ asOfTags("LEFT", nullStruct, ">=", s"($nullStruct, 'rnull'), ($one, 'r1')"),
+ Row(null) :: Nil)
+ }
+ }
+ }
+
+ gridTest("whole STRUCT column MATCH_CONDITION matches a struct whose fields are all NULL")(Seq(
+ ">=" -> Seq(Row("allnull")), "<=" -> Seq(Row("allnull")), ">" -> Nil, "<" -> Nil)) {
+ case (op, expected) =>
+ // Two structs of NULL fields are equal, and a NULL struct never matches.
+ val allNull = "named_struct('a', CAST(NULL AS INT), 'b', CAST(NULL AS INT))"
+ val nullStruct = "CAST(NULL AS STRUCT<a: INT, b: INT>)"
+ Seq(true, false).foreach { ansiEnabled =>
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ checkSortMergeAsOf(
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES ($allNull, 'l') AS t(k, tag)
+ |ASOF JOIN VALUES ($nullStruct, 'rnull'), ($allNull, 'allnull') AS r(k, tag)
+ | MATCH_CONDITION (t.k $op r.k)
+ |""".stripMargin),
+ expected)
+ }
+ }
+ }
+
+ test("nested STRUCT column MATCH_CONDITION keeps a NULL inner struct apart from NULL fields") {
+ // {e: NULL} sorts before {e: {a: NULL}}, as in a plain comparison.
+ val eNull = "named_struct('e', CAST(NULL AS STRUCT<a: INT>))"
+ val aNull = "named_struct('e', named_struct('a', CAST(NULL AS INT)))"
+ for {
+ (left, op, right) <- Seq((aNull, ">", eNull), (eNull, "<", aNull))
+ ansiEnabled <- Seq(true, false)
+ } {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
+ checkSortMergeAsOf(
+ sql(
+ s"""
+ |SELECT r.tag
+ |FROM VALUES ($left, 'l') AS t(k, tag)
+ |ASOF JOIN VALUES ($right, 'r') AS r(k, tag)
+ | MATCH_CONDITION (t.k $op r.k)
+ |""".stripMargin),
+ Row("r") :: Nil)
+ }
+ }
+ }
+
+ test("tuple vs nullable STRUCT column MATCH_CONDITION: a NULL struct does not end the scan") {
+ // A NULL struct and {NULL, NULL} have the same fields, so a sort by fields ties them. Both
Review Comment:
**Nit (P3):** This comment does not match what the test exercises. The right operand `r.s` is a whole struct column, so `matchSortKeys` keeps it as a single sort value (only a SQL tuple literal splits into per-field keys). Sorted NullsFirst, the NULL struct (`rnull`) sorts strictly before the all-NULL-fields struct (`allnull`) -- as the production comment puts it, "a NULL struct comes before a struct of NULL fields" -- so they do not tie, and the NULL row sorts first, not after a match. The "sort by fields ties them" reasoning only applies to the decomposed left tuple, as the adjacent test at line 553 notes. Consider rewording to describe the whole-struct ordering and the actual reason both right-side input orders are exercised.
**Verification:**
- **Inspection:** Read the revised comment and confirm it describes whole-struct ordering (NULL struct before all-NULL-fields struct, no tie) consistent with matchSortKeys and the production comment in basicLogicalOperators.scala.
- **Regression:** Run the 'tuple vs nullable STRUCT column MATCH_CONDITION' test and confirm it still passes with the unchanged assertions, so the comment edit did not alter behavior.
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "SavicStefan (via GitHub)" <gi...@apache.org>.
SavicStefan commented on code in PR #59134:
URL: https://github.com/apache/spark/pull/59134#discussion_r4165149985
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2890,8 +2892,10 @@ object AsOfJoin {
(leftOperand.dataType, rightOperand.dataType) match {
case (leftStruct: StructType, rightStruct: StructType)
if MatchConditionTypes.usesIdenticalStructSort(leftStruct, rightStruct) =>
- if (isSqlTupleStructOperand(leftOperand) || isSqlTupleStructOperand(rightOperand)) {
- val pairs = collectStructLeafPairs(leftOperand, rightOperand, leftStruct)
+ if ((isSqlTupleStructOperand(leftOperand) || isSqlTupleStructOperand(rightOperand)) &&
+ !leftOperand.nullable && !rightOperand.nullable) {
Review Comment:
is there a reason `SortMergeAsOfJoinExec` requires the same number of sort keys on both sides, because like If one operand is a SQL tuple and the other is a STRUCT column, could we just return separate field expressions for that tuple and one whole struct key for the column?
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on code in PR #59134:
URL: https://github.com/apache/spark/pull/59134#discussion_r4165886287
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -3105,17 +3091,21 @@ object AsOfJoin {
wrapCompositeOrderExpression(leafDiffs)
}
+ /** `splitNullableStructs = false` keeps a struct pair whole when either side can be NULL. */
private def collectStructLeafPairs(
leftOperand: Expression,
rightOperand: Expression,
- structType: StructType): Seq[(Expression, Expression)] = {
+ structType: StructType,
+ splitNullableStructs: Boolean = true): Seq[(Expression, Expression)] = {
structFieldExprs(leftOperand, structType)
.zip(structFieldExprs(rightOperand, structType))
.flatMap {
case (left, right) =>
- if (MatchConditionTypes.usesStructDecomposition(left.dataType, right.dataType)) {
+ val canSplit = splitNullableStructs || (!left.nullable && !right.nullable)
+ if (canSplit &&
+ MatchConditionTypes.usesStructDecomposition(left.dataType, right.dataType)) {
Review Comment:
Agreed, changed.
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on code in PR #59134:
URL: https://github.com/apache/spark/pull/59134#discussion_r4166292195
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2890,8 +2892,10 @@ object AsOfJoin {
(leftOperand.dataType, rightOperand.dataType) match {
case (leftStruct: StructType, rightStruct: StructType)
if MatchConditionTypes.usesIdenticalStructSort(leftStruct, rightStruct) =>
- if (isSqlTupleStructOperand(leftOperand) || isSqlTupleStructOperand(rightOperand)) {
- val pairs = collectStructLeafPairs(leftOperand, rightOperand, leftStruct)
+ if ((isSqlTupleStructOperand(leftOperand) || isSqlTupleStructOperand(rightOperand)) &&
+ !leftOperand.nullable && !rightOperand.nullable) {
Review Comment:
Yes, you are right, I was talking result wise. We will keep splitting the tuples without condition since they are not-null always, and for structs we are going to keep them whole as that catches the null and non-null structs as they should be. Thanks a lot, code looks much cleaner and logic much better.
--
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-59858][SQL] Compare ASOF JOIN STRUCT operands without rebuilding them [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #59134:
URL: https://github.com/apache/spark/pull/59134#issuecomment-6016759022
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/43ccc6baa6e55b250faddae0e0a25a431337309c
- merged into branch-4.x https://github.com/apache/spark/commit/028e3f154dc0b5dd69636770cdc7bd132039c9e4
*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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "SavicStefan (via GitHub)" <gi...@apache.org>.
SavicStefan commented on code in PR #59134:
URL: https://github.com/apache/spark/pull/59134#discussion_r4165957225
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2890,8 +2892,10 @@ object AsOfJoin {
(leftOperand.dataType, rightOperand.dataType) match {
case (leftStruct: StructType, rightStruct: StructType)
if MatchConditionTypes.usesIdenticalStructSort(leftStruct, rightStruct) =>
- if (isSqlTupleStructOperand(leftOperand) || isSqlTupleStructOperand(rightOperand)) {
- val pairs = collectStructLeafPairs(leftOperand, rightOperand, leftStruct)
+ if ((isSqlTupleStructOperand(leftOperand) || isSqlTupleStructOperand(rightOperand)) &&
+ !leftOperand.nullable && !rightOperand.nullable) {
Review Comment:
we gain simpler logic, because we are choosing sort keys independently for each side, splitting tuples where safe and keeping STRUCT columns whole and we since equal key counts isn't required we could remove that restriction
--
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-59858][SQL] Keep NULL STRUCT operands NULL in ASOF JOIN MATCH_CONDITION [spark]
Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on code in PR #59134:
URL: https://github.com/apache/spark/pull/59134#discussion_r4167441213
##########
sql/core/src/test/scala/org/apache/spark/sql/AsOfJoinSortMergeSQLSuite.scala:
##########
@@ -440,6 +448,117 @@ class AsOfJoinSortMergeSQLSuite extends QueryTest
Row(Timestamp.valueOf("2026-06-29 10:03:00"), 1, "v1.1")))
}
+ test("NULL whole STRUCT column MATCH_CONDITION never matches") {
Review Comment:
Added, thanks!
--
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-59858][SQL] Compare ASOF JOIN STRUCT operands without rebuilding them [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b closed pull request #59134: [SPARK-59858][SQL] Compare ASOF JOIN STRUCT operands without rebuilding them
URL: https://github.com/apache/spark/pull/59134
--
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-59858][SQL] Compare ASOF JOIN STRUCT operands without rebuilding them [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #59134:
URL: https://github.com/apache/spark/pull/59134#issuecomment-6016730427
Thank you @LukaZdravic and @SavicStefan @vranes @HyukjinKwon!
--
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