vranes commented on code in PR #59322:
URL: https://github.com/apache/spark/pull/59322#discussion_r4231522977
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2628,6 +2628,7 @@ object LateralJoin {
/**
* A logical plan for as-of join.
+ * @param direction Which matching right row to keep.
Review Comment:
The exec now picks the scan from `direction`, but it still filters with
`asOfCondition` and measures distance with `orderExpression`. These must match,
so we should probably document this
```suggestion
* @param direction Which matching right row to keep: the last for
[[Backward]], the first for
* [[Forward]], the closest by `orderExpression` for
[[Nearest]].
* `asOfCondition` and `orderExpression` must be built
for the same direction.
```
##########
sql/core/src/test/scala/org/apache/spark/sql/AsOfJoinSortMergeSQLSuite.scala:
##########
@@ -212,6 +213,39 @@ class AsOfJoinSortMergeSQLSuite extends QueryTest
Row(180.15) :: Nil)
}
+ test("MATCH_CONDITION with the right operand written first plans the flipped
direction") {
Review Comment:
This repeats the two existing tests above (same queries, same rows), and
dual-run already compares both analyzers. Can we move the direction assert
there and drop this one?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveAsOfJoin.scala:
##########
@@ -160,6 +163,7 @@ private[analysis] object AsOfJoinMatchConditionResolution {
Some(MaterializedMatchCondition(
asOfCondition = asOfCondition,
orderExpression = orderExpression,
+ direction = normalizedOperator.direction,
Review Comment:
Nit: the object doc above still says this rule materializes MATCH_CONDITION
into `asOfCondition` and `orderExpression` only. Maybe point it to
`MaterializedMatchCondition` instead, so it stays up to date
##########
sql/core/src/test/scala/org/apache/spark/sql/SortMergeAsOfJoinSuite.scala:
##########
@@ -158,6 +187,39 @@ class SortMergeAsOfJoinSuite extends QueryTest
)
}
+ test("keeps the requested direction when a rewrite puts the right side
first") {
+ val schema = StructType(
+ StructField("ts", IntegerType) ::
+ StructField("val", StringType) :: Nil)
+ val left = spark.createDataFrame(List(Row(10, "a")).asJava, schema)
+ // Pairs of right rows tie on ts. Backward keeps the last row of a tie,
Nearest the first.
Review Comment:
Nit: for the forward case, Forward and Nearest both return r3 on this data,
so only the `op.direction` assert checks it, so ties are not that relevant.
Remove the comment or make it clearer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
