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/24 12:20:14 UTC

[PR] [SPARK-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

LukaZdravic opened a new pull request, #59016:
URL: https://github.com/apache/spark/pull/59016

   ### What changes were proposed in this pull request?
   
   ASOF JOIN `MATCH_CONDITION` now accepts operands that are, or contain, an empty struct (`STRUCT<>`). The operand check was "orderable and contains no empty struct". It is now only "orderable", the same rule `min_by` and `max_by` use for their ordering column.
   
   Now accepted (same type on both sides): `STRUCT<>`, `STRUCT<x: STRUCT<>>`, `ARRAY<STRUCT<>>`. An empty struct against a non-empty struct stays rejected.
   
   ### Why are the changes needed?
   
   The comparison operator already accepts these pairs: `named_struct() >= named_struct()` is `true`, because two empty structs are equal. ASOF rejected them with an error that says the operands must be orderable, but `STRUCT<>` is orderable.
   
   ### Does this PR introduce _any_ user-facing change?
   
   Yes. This query failed and now runs:
   
   ```sql
   SELECT * FROM VALUES (named_struct()) AS l(s)
   ASOF JOIN VALUES (named_struct()) AS r(s)
   MATCH_CONDITION (l.s >= r.s);
   -- before: [ASOF_JOIN_MATCH_CONDITION_INVALID_TYPE] ... Got: "STRUCT<>" and "STRUCT<>".
   ```
   
   Two empty structs are equal, so `>=` and `<=` match, and `>` and `<` find no match. SQL ASOF JOIN is not in a released Spark version yet.
   
   ### How was this patch tested?
   
   Unit tests in `AsOfJoinMatchConditionTypesSuite` and `AsOfJoinSQLSuite` for the accepted and rejected type pairs. Execution tests in `AsOfJoinSortMergeSQLSuite` assert the matched row for all four operators, for: an empty struct, a struct with only an empty field, a struct with an empty field and an int field (column and tuple form), and arrays of empty structs.
   
   ### 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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #59016:
URL: https://github.com/apache/spark/pull/59016#issuecomment-5911846909

   Thank you @LukaZdravic @vranes!


-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "vranes (via GitHub)" <gi...@apache.org>.
vranes commented on code in PR #59016:
URL: https://github.com/apache/spark/pull/59016#discussion_r4124420485


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeAsOfJoinExec.scala:
##########
@@ -424,10 +405,38 @@ private[joins] class SortMergeAsOfJoinScanner(
   }
 
   /**
-   * Forward scan for Forward/Nearest joins: distance-based termination.
-   * Stop when distance starts increasing past the minimum found so far.
+   * Forward joins: returns the first row that passes both conditions. It picks by buffer
+   * order, not by distance: the distance can rank STRUCT and ARRAY values wrongly, for

Review Comment:
   Again, we don't need historical / iterative style comments. And `findFirstForward` and `findLastBackward` comments should be aligned.



##########
docs/sql-ref-syntax-qry-select-asof-join.md:
##########
@@ -121,7 +121,9 @@ comparison_operator
 
 * **NULL.** A `NULL` in either operand never satisfies the `MATCH_CONDITION`
   comparison. Left rows whose operand is `NULL` are dropped under `INNER ASOF` and
-  retained with `NULL` right-side columns under `LEFT ASOF`.
+  retained with `NULL` right-side columns under `LEFT ASOF`. A `NULL` element of an
+  `ARRAY` operand, or a `NULL` field of a `STRUCT` operand, does not block a match. It
+  sorts before any non-`NULL` value, so `ARRAY(NULL) <= ARRAY(5)` is true.

Review Comment:
   Is this true in Spark generally?



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2772,8 +2772,7 @@ object AsOfJoin {
    */
   private[catalyst] object MatchConditionTypes {
 
-    def isValidOperandType(dataType: DataType): Boolean =
-      RowOrdering.isOrderable(dataType) && !containsEmptyStructType(dataType)
+    def isValidOperandType(dataType: DataType): Boolean = RowOrdering.isOrderable(dataType)

Review Comment:
   Now for STRUCT<e: STRUCT<>>, a NULL value becomes a non-NULL struct with e = NULL. As a result, two NULL values can match with >= . Is that behaviour desired here? 



-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "vranes (via GitHub)" <gi...@apache.org>.
vranes commented on code in PR #59016:
URL: https://github.com/apache/spark/pull/59016#discussion_r4124811692


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2772,8 +2772,7 @@ object AsOfJoin {
    */
   private[catalyst] object MatchConditionTypes {
 
-    def isValidOperandType(dataType: DataType): Boolean =
-      RowOrdering.isOrderable(dataType) && !containsEmptyStructType(dataType)
+    def isValidOperandType(dataType: DataType): Boolean = RowOrdering.isOrderable(dataType)

Review Comment:
   Plus double check whether this bug was preexisting for non-nested structs too before this PR. 



-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on code in PR #59016:
URL: https://github.com/apache/spark/pull/59016#discussion_r4131635089


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2772,8 +2772,7 @@ object AsOfJoin {
    */
   private[catalyst] object MatchConditionTypes {
 
-    def isValidOperandType(dataType: DataType): Boolean =
-      RowOrdering.isOrderable(dataType) && !containsEmptyStructType(dataType)
+    def isValidOperandType(dataType: DataType): Boolean = RowOrdering.isOrderable(dataType)

Review Comment:
   So it's an asof specific problem and it's about how the match condition values are re-written. Both for nested and non-nested structs problem is the same they get re-written as a new struct built from their fields, so a NULL struct stops being NULL. 
   With this PR, structs that contain an empty struct can now reach this:
   
   - `STRUCT<e: STRUCT<>>`: NULL becomes `{e: NULL}`
   - `STRUCT<a: INT, e: STRUCT<>>`: NULL becomes `{a: NULL, e: NULL}`
   
   This is not new with this PR. Master already does the same for normal (non-empty) structs:
   
   - `STRUCT<a: INT>`: NULL becomes `{a: NULL}`
   - `STRUCT<e: STRUCT<a: INT>>`: NULL becomes `{col1: {a: NULL}}`
   
   Because of that, `NULL >= NULL` matches in ASOF, while a plain `WHERE` with the same comparison does not.



-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on code in PR #59016:
URL: https://github.com/apache/spark/pull/59016#discussion_r4133285332


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2772,8 +2772,7 @@ object AsOfJoin {
    */
   private[catalyst] object MatchConditionTypes {
 
-    def isValidOperandType(dataType: DataType): Boolean =
-      RowOrdering.isOrderable(dataType) && !containsEmptyStructType(dataType)
+    def isValidOperandType(dataType: DataType): Boolean = RowOrdering.isOrderable(dataType)

Review Comment:
   Here is the new PR https://github.com/apache/spark/pull/59134, one thing to mention, it doesn't contain yet the test for forward matching because I am waiting for https://github.com/apache/spark/pull/59102 to merge which fixes the not needed distance calculation for forward join. It says the same in the description, I just tought it's worth mentioning here too.



-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on PR #59016:
URL: https://github.com/apache/spark/pull/59016#issuecomment-5875272721

   @vranes Thanks for the review. I moved the forward-match change to its own PR, #59102 (SPARK-59829), because it also fixes an overflow bug. Your suggestions on that code went with it. This PR now only covers empty STRUCT operands and the mixed-type ARRAY<STRUCT> fix.


-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "vranes (via GitHub)" <gi...@apache.org>.
vranes commented on code in PR #59016:
URL: https://github.com/apache/spark/pull/59016#discussion_r4143074146


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2772,8 +2772,7 @@ object AsOfJoin {
    */
   private[catalyst] object MatchConditionTypes {
 
-    def isValidOperandType(dataType: DataType): Boolean =
-      RowOrdering.isOrderable(dataType) && !containsEmptyStructType(dataType)
+    def isValidOperandType(dataType: DataType): Boolean = RowOrdering.isOrderable(dataType)

Review Comment:
   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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "vranes (via GitHub)" <gi...@apache.org>.
vranes commented on code in PR #59016:
URL: https://github.com/apache/spark/pull/59016#discussion_r4124811692


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2772,8 +2772,7 @@ object AsOfJoin {
    */
   private[catalyst] object MatchConditionTypes {
 
-    def isValidOperandType(dataType: DataType): Boolean =
-      RowOrdering.isOrderable(dataType) && !containsEmptyStructType(dataType)
+    def isValidOperandType(dataType: DataType): Boolean = RowOrdering.isOrderable(dataType)

Review Comment:
   + Double check whether this bug was preexisting for non-nested structs too before this PR. 



-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on code in PR #59016:
URL: https://github.com/apache/spark/pull/59016#discussion_r4133285332


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2772,8 +2772,7 @@ object AsOfJoin {
    */
   private[catalyst] object MatchConditionTypes {
 
-    def isValidOperandType(dataType: DataType): Boolean =
-      RowOrdering.isOrderable(dataType) && !containsEmptyStructType(dataType)
+    def isValidOperandType(dataType: DataType): Boolean = RowOrdering.isOrderable(dataType)

Review Comment:
   https://github.com/apache/spark/pull/59134, one thing to mention, it doesn't contain yet the test for forward matching because I am waiting for https://github.com/apache/spark/pull/59102 to merge which fixes the not needed distance calculation for forward join. It says the same in the description, I just tought it's worth mentioning here too.



-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "vranes (via GitHub)" <gi...@apache.org>.
vranes commented on code in PR #59016:
URL: https://github.com/apache/spark/pull/59016#discussion_r4116301564


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeAsOfJoinExec.scala:
##########
@@ -94,32 +94,36 @@ case class SortMergeAsOfJoinExec(
 
   // Backward joins scan the right-side buffer forward and keep the last match.
   // Detect from asOfCondition so composite MATCH_CONDITION sort keys still work.
-  private val isBackwardJoin: Boolean =
-    isBackwardAsOfCondition(asOfCondition, left.output, right.output)
-
-  private def isBackwardAsOfCondition(
-      condition: Expression,
-      leftOutput: Seq[Attribute],
-      rightOutput: Seq[Attribute]): Boolean = {
-    val leftAttrs = AttributeSet(leftOutput)
-    val rightAttrs = AttributeSet(rightOutput)
-    def isBackwardPredicate(expr: Expression): Boolean = expr match {
-      case GreaterThanOrEqual(l, r)
-          if l.references.subsetOf(leftAttrs) && r.references.subsetOf(rightAttrs) =>
-        true
-      case GreaterThan(l, r)
-          if l.references.subsetOf(leftAttrs) && r.references.subsetOf(rightAttrs) =>
-        true
-      case And(c, _) => isBackwardPredicate(c)
-      case _ => false
+  private val isBackwardJoin: Boolean = leadingAsOfComparison.exists {

Review Comment:
   Could we derive one AsOfJoinDirection here and pass it to the scanner? That would avoid duplicating the check and make `nearest` explicit instead of representing it as two false values



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -3123,16 +3115,11 @@ object AsOfJoin {
       }
   }
 
-  private def wrapCompositeOrderExpression(
-      diffs: Seq[Expression],
-      compositeType: DataType): Expression = {
+  /** A struct, not an array, since field distances can differ in type (INT vs INTERVAL). */

Review Comment:
   ```suggestion
     /** Groups multiple field distances in a struct because their types may differ. */
   ```



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2772,8 +2772,8 @@ object AsOfJoin {
    */
   private[catalyst] object MatchConditionTypes {
 
-    def isValidOperandType(dataType: DataType): Boolean =
-      RowOrdering.isOrderable(dataType) && !containsEmptyStructType(dataType)
+    /** Any orderable type, including an empty struct, which equals any other empty struct. */

Review Comment:
   nit: unnecessary comment



##########
sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeAsOfJoinExec.scala:
##########
@@ -424,10 +436,40 @@ private[joins] class SortMergeAsOfJoinScanner(
   }
 
   /**
-   * Forward scan for Forward/Nearest joins: distance-based termination.

Review Comment:
   nit: This repeats nearby documentation and could be shorter



##########
sql/core/src/test/scala/org/apache/spark/sql/AsOfJoinSortMergeSQLSuite.scala:
##########
@@ -435,6 +435,67 @@ class AsOfJoinSortMergeSQLSuite extends QueryTest
       Row(Row(5, Seq(1, 1))) :: Nil)
   }
 

Review Comment:
   Add a forward test that makes the closest right row fail the ON condition and a later row pass



##########
sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeAsOfJoinExec.scala:
##########
@@ -156,7 +160,10 @@ case class SortMergeAsOfJoinExec(
  * last as-of-satisfying row as the best match (since the buffer is
  * sorted ascending, the last satisfying row is the closest).
  *
- * For Forward/Nearest joins, the forward scan uses distance-based
+ * For Forward joins (left.t <= right.t), the first as-of-satisfying row
+ * is the match (the buffer is sorted ascending, so it is the smallest).
+ *
+ * For Nearest joins, the forward scan uses distance-based

Review Comment:
   nit: If this comment describes Forward and Nearest, why does it leave out Backward?



##########
sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeAsOfJoinExec.scala:
##########
@@ -424,10 +436,40 @@ private[joins] class SortMergeAsOfJoinScanner(
   }
 
   /**
-   * Forward scan for Forward/Nearest joins: distance-based termination.

Review Comment:
   Also, `findFirstForward` and `findLastBackward` comment style should be aligned.



##########
sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeAsOfJoinExec.scala:
##########
@@ -365,14 +373,18 @@ private[joins] class SortMergeAsOfJoinScanner(
    * right.t <= left.t, which is the closest match). Early-terminates
    * when as-of condition transitions from true to false (monotone).
    *
-   * For Forward/Nearest joins: uses distance-based early termination
+   * For Forward joins: returns the first as-of-satisfying row.
+   *
+   * For Nearest joins: uses distance-based early termination
    * (stop when distance starts increasing past the minimum).
    */
   private def findBestInGroup(leftRow: InternalRow): InternalRow = {
     if (isBackwardJoin) {
       findBestBackwardForward(leftRow)

Review Comment:
   Rename to `findLastBackward` to match `findFirstForward`. The third one should be `findNearest`.



##########
sql/core/src/test/scala/org/apache/spark/sql/AsOfJoinSortMergeSQLSuite.scala:
##########
@@ -728,6 +807,74 @@ class AsOfJoinSortMergeSQLSuite extends QueryTest
       Row(Seq(1, 6)) :: Nil)
   }
 
+  test("forward MATCH_CONDITION with NULL array elements picks the smallest right row") {
+    // NULL elements sort first: [null] < [null, null] < [5]. A distance-based pick chose [5].

Review Comment:
   nit: Historical / iterative comment. This belongs more to the PR description, not in the code.



##########
sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeAsOfJoinExec.scala:
##########
@@ -156,7 +160,10 @@ case class SortMergeAsOfJoinExec(
  * last as-of-satisfying row as the best match (since the buffer is
  * sorted ascending, the last satisfying row is the closest).
  *
- * For Forward/Nearest joins, the forward scan uses distance-based
+ * For Forward joins (left.t <= right.t), the first as-of-satisfying row
+ * is the match (the buffer is sorted ascending, so it is the smallest).
+ *
+ * For Nearest joins, the forward scan uses distance-based

Review Comment:
   Same below, plus these 2 comments are repeating themselves 



-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "vranes (via GitHub)" <gi...@apache.org>.
vranes commented on code in PR #59016:
URL: https://github.com/apache/spark/pull/59016#discussion_r4132178381


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2772,8 +2772,7 @@ object AsOfJoin {
    */
   private[catalyst] object MatchConditionTypes {
 
-    def isValidOperandType(dataType: DataType): Boolean =
-      RowOrdering.isOrderable(dataType) && !containsEmptyStructType(dataType)
+    def isValidOperandType(dataType: DataType): Boolean = RowOrdering.isOrderable(dataType)

Review Comment:
   Thanks, please link the PR with the fix here when published.



-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #59016:
URL: https://github.com/apache/spark/pull/59016#issuecomment-5911929375

   **Merge Summary:**
   - merged into master https://github.com/apache/spark/commit/6f0c4791a1f4bceaa9749c913dc80cef47918a5b
   - merged into branch-4.x https://github.com/apache/spark/commit/22f2aff31ba8b4c44ce72eb1e767dec400e32144
   
   *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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on PR #59016:
URL: https://github.com/apache/spark/pull/59016#issuecomment-5909427048

   > Mostly looks good, leaving a few comments below:
   > 
   > 1. Nested empty structs still hit the CreateStruct NULL rewrite (related, not new logic)
   > 
   > STRUCT<e: STRUCT<>> is newly accepted and is decomposed. alignOperandsForComparison rebuilds it as CreateStruct(GetStructField(…)), so a NULL operand becomes {e: NULL} and NULL >= NULL matches. Top-level STRUCT<> avoids that on purpose; the nested form does not.
   > 
   > The new NULL test only uses CAST(NULL AS STRUCT<>). Please make sure #59134 covers CAST(NULL AS STRUCT<e: STRUCT<>>) as well. I would not block this PR on that if #59134 is the agreed follow-up.
   > 
   > 2. Golden FVT-ASOF-4-018b does not test what the comment says
   > 
   > The comment is “elements tie, so length decides”, but the right side is only length 1 and 3. Left length 2 with >= matches only the shorter array. That does not show length as a tie-break among several matches.
   > 
   > The Scala test already has the right shape (lengths 1, 2, and 3, all four operators). Mirror that in join-asof-datatypes.sql.
   > 
   > 3. The mixed-type ARRAY fix has no analyzer unit test
   > 
   > AsOfJoinSortMergeSQLSuite is a good regression test, but the original failure was analysis-time CreateArray type-checking. ResolveAsOfJoinSuite already pins empty-array ZipWith; a sibling case for ARRAY<STRUCT<f: INT, ts: TIMESTAMP>> asserting the lambda body is CreateNamedStruct (not CreateArray) would lock the fix cheaply.
   
   Thank you! Addressed 2 and 3: FVT-ASOF-4-018b now has lengths 1, 2 and 3 with all four operators (4-018b to 4-018e), and ResolveAsOfJoinSuite checks that a mixed-type ARRAY<STRUCT> gets a CreateNamedStruct element distance. For 1, #59134 will add CAST(NULL AS STRUCT<e: STRUCT<>>) once this PR merges, since master rejects that type until then.


-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b closed pull request #59016: [SPARK-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION
URL: https://github.com/apache/spark/pull/59016


-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on code in PR #59016:
URL: https://github.com/apache/spark/pull/59016#discussion_r4131894513


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeAsOfJoinExec.scala:
##########
@@ -424,10 +405,38 @@ private[joins] class SortMergeAsOfJoinScanner(
   }
 
   /**
-   * Forward scan for Forward/Nearest joins: distance-based termination.
-   * Stop when distance starts increasing past the minimum found so far.
+   * Forward joins: returns the first row that passes both conditions. It picks by buffer
+   * order, not by distance: the distance can rank STRUCT and ARRAY values wrongly, for

Review Comment:
   Thanks, will fix



-- 
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-59749][SQL] Accept empty STRUCT operands in ASOF JOIN MATCH_CONDITION [spark]

Posted by "LukaZdravic (via GitHub)" <gi...@apache.org>.
LukaZdravic commented on code in PR #59016:
URL: https://github.com/apache/spark/pull/59016#discussion_r4131325081


##########
docs/sql-ref-syntax-qry-select-asof-join.md:
##########
@@ -121,7 +121,9 @@ comparison_operator
 
 * **NULL.** A `NULL` in either operand never satisfies the `MATCH_CONDITION`
   comparison. Left rows whose operand is `NULL` are dropped under `INNER ASOF` and
-  retained with `NULL` right-side columns under `LEFT ASOF`.
+  retained with `NULL` right-side columns under `LEFT ASOF`. A `NULL` element of an
+  `ARRAY` operand, or a `NULL` field of a `STRUCT` operand, does not block a match. It
+  sorts before any non-`NULL` value, so `ARRAY(NULL) <= ARRAY(5)` is true.

Review Comment:
   Yes, it's general Spark 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