vranes commented on code in PR #58823:
URL: https://github.com/apache/spark/pull/58823#discussion_r4027612733
##########
sql/core/src/test/resources/sql-tests/inputs/join-asof-datatypes.sql:
##########
@@ -127,6 +127,25 @@ FROM VALUES (TIMESTAMP '2026-06-29 10:00:00') AS t(ts)
ASOF JOIN
VALUES (TIMESTAMP_NTZ '2026-06-29 09:00:00') AS r(ts_ntz)
MATCH_CONDITION (t.ts >= r.ts_ntz);
+-- FVT-ASOF-4-016a: coercion DATE vs STRING (SPARK-59527), coerced like the >=
operator
+SELECT t.d, r.s AS matched_s
+FROM VALUES (DATE '2026-06-29') AS t(d) ASOF JOIN
+ VALUES ('2026-06-28'), ('2026-06-29') AS r(s)
+ MATCH_CONDITION (t.d >= r.s);
+
+-- FVT-ASOF-4-016b: coercion TIMESTAMP vs STRING (SPARK-59527)
+SELECT t.ts, r.s AS matched_s
+FROM VALUES (TIMESTAMP '2026-06-29 10:00:00') AS t(ts) ASOF JOIN
+ VALUES ('2026-06-29 09:00:00'), ('2026-06-29 10:00:00') AS r(s)
+ MATCH_CONDITION (t.ts >= r.s);
+
+-- FVT-ASOF-4-016c: coercion INT vs STRING sorts the right buffer by value,
not lexicographically.
+-- '9' sorts after '10'/'20' as text but 9 < 10 < 20 by value; the as-of match
for 20 must be 20.
+SELECT t.k, r.s AS matched_s
+FROM VALUES (20) AS t(k) ASOF JOIN
Review Comment:
IMO changing the left-side value to 25 would make the tested behavior more
obvious to readers
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/logical/AsOfJoinMatchConditionTypesSuite.scala:
##########
@@ -30,18 +29,43 @@ class AsOfJoinMatchConditionTypesSuite extends
SparkFunSuite {
assert(!MatchConditionTypes.usesStructDecomposition(IntegerType, LongType))
}
- test("string and temporal types are incompatible") {
- assert(!MatchConditionTypes.areOperandsCompatible(StringType,
TimestampType))
- assert(!MatchConditionTypes.areOperandsCompatible(DateType, StringType))
+ test("scalar string and temporal types coerce like the comparison operator")
{
+ assert(MatchConditionTypes.areOperandsCompatible(StringType,
TimestampType))
+ assert(MatchConditionTypes.areOperandsCompatible(DateType, StringType))
+ // Common type is the temporal type (string cast to it), so sort and
comparison agree.
+ assert(MatchConditionTypes.matchComparisonCommonType(DateType,
StringType).contains(DateType))
+ assert(
+ MatchConditionTypes.matchComparisonCommonType(StringType, TimestampType)
+ .contains(TimestampType))
}
- test("orderable scalars with no common type are incompatible") {
- // Both are individually valid, orderable operands ...
- assert(MatchConditionTypes.isValidOperandType(TimestampType))
- assert(MatchConditionTypes.isValidOperandType(BooleanType))
- // ... but TIMESTAMP and BOOLEAN have no common wider type and are not a
- // string/temporal pair, so they are not comparable.
- assert(!MatchConditionTypes.areOperandsCompatible(TimestampType,
BooleanType))
+ test("scalar string and numeric types coerce like the comparison operator") {
+ // Common type must be numeric, not string, so the buffer sorts by value.
+ assert(MatchConditionTypes.areOperandsCompatible(IntegerType, StringType))
+ assert(MatchConditionTypes.matchComparisonCommonType(IntegerType,
StringType).nonEmpty)
+ assert(
+ !MatchConditionTypes.matchComparisonCommonType(IntegerType,
StringType).contains(StringType))
+ }
+
+ test("scalar string vs interval is rejected, matching the comparison
operator") {
+ // No comparison common type exists, so reject it like `>=` rather than
leave it uncoerced.
Review Comment:
nit:
```suggestion
// No comparison common type exists, so reject it instead of leaving it
uncoerced.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2749,7 +2728,62 @@ object AsOfJoin {
def isValidOperandType(dataType: DataType): Boolean =
RowOrdering.isOrderable(dataType) && !containsEmptyStructType(dataType)
+ /**
+ * Top-level operand compatibility. A string vs non-string scalar pair is
compatible only when
+ * the comparison operator has a common type for it, so it coerces like
`>=` (accepting DATE vs
+ * STRING, rejecting STRING vs INTERVAL). Other scalars widen;
STRUCT/ARRAY use the stricter
+ * [[areFieldTypesCompatible]] rule.
+ */
def areOperandsCompatible(leftType: DataType, rightType: DataType):
Boolean = {
+ if (!isValidOperandType(leftType) || !isValidOperandType(rightType)) {
+ false
+ } else if (isCompositeOperand(leftType, rightType)) {
+ areFieldTypesCompatible(leftType, rightType)
+ } else if (isExactlyOneStringPair(leftType, rightType)) {
+ matchComparisonCommonType(leftType, rightType).isDefined
+ } else {
+ TypeCoercion.findWiderTypeForTwo(leftType, rightType).isDefined
+ }
+ }
+
+ /**
+ * The type a string vs non-string scalar pair is cast to for comparison,
ordering, and sort,
+ * mirroring the comparison operator's string coercion in the active mode
(ANSI or default).
+ * Returns [[None]] for other pairs, whose monotonic widening keeps the
raw operands.
+ */
+ private[catalyst] def matchComparisonCommonType(
+ leftType: DataType,
+ rightType: DataType): Option[DataType] = {
+ if (!isExactlyOneStringPair(leftType, rightType)) {
+ None
+ } else {
+ val commonType = if (SQLConf.get.ansiEnabled) {
+ AnsiStringPromotionTypeCoercion.findWiderTypeForString(leftType,
rightType)
+ } else {
+ TypeCoercion.findCommonTypeForBinaryComparison(leftType, rightType,
SQLConf.get)
+ }
+ commonType.filter(isValidOperandType)
+ }
+ }
+
+ /**
+ * True when either operand is a STRUCT or ARRAY. These keep the stricter
widening rule and are
+ * left uncoerced; [[areOperandsCompatible]] and the leaf coercion both
branch on this.
+ */
+ private[catalyst] def isCompositeOperand(
+ leftType: DataType,
+ rightType: DataType): Boolean =
+ Seq(leftType, rightType).exists(t => t.isInstanceOf[StructType] ||
t.isInstanceOf[ArrayType])
+
+ /** True when exactly one operand is a string, the only pair that needs
comparison coercion. */
+ private def isExactlyOneStringPair(leftType: DataType, rightType:
DataType): Boolean =
+ leftType.isInstanceOf[StringType] != rightType.isInstanceOf[StringType]
Review Comment:
[Optional suggestion] Move `isCompositeOperand` and `isExactlyOneStringPair`
above matchComparisonCommonType so they are in the order of reference in
`areOperandsCompatible`
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/logical/AsOfJoinMatchConditionTypesSuite.scala:
##########
@@ -30,18 +29,43 @@ class AsOfJoinMatchConditionTypesSuite extends
SparkFunSuite {
assert(!MatchConditionTypes.usesStructDecomposition(IntegerType, LongType))
}
- test("string and temporal types are incompatible") {
- assert(!MatchConditionTypes.areOperandsCompatible(StringType,
TimestampType))
- assert(!MatchConditionTypes.areOperandsCompatible(DateType, StringType))
+ test("scalar string and temporal types coerce like the comparison operator")
{
+ assert(MatchConditionTypes.areOperandsCompatible(StringType,
TimestampType))
+ assert(MatchConditionTypes.areOperandsCompatible(DateType, StringType))
+ // Common type is the temporal type (string cast to it), so sort and
comparison agree.
+ assert(MatchConditionTypes.matchComparisonCommonType(DateType,
StringType).contains(DateType))
+ assert(
+ MatchConditionTypes.matchComparisonCommonType(StringType, TimestampType)
+ .contains(TimestampType))
}
- test("orderable scalars with no common type are incompatible") {
- // Both are individually valid, orderable operands ...
- assert(MatchConditionTypes.isValidOperandType(TimestampType))
- assert(MatchConditionTypes.isValidOperandType(BooleanType))
- // ... but TIMESTAMP and BOOLEAN have no common wider type and are not a
- // string/temporal pair, so they are not comparable.
- assert(!MatchConditionTypes.areOperandsCompatible(TimestampType,
BooleanType))
+ test("scalar string and numeric types coerce like the comparison operator") {
Review Comment:
nit: change the title of this case (and the one above) to match the style of
scalar string vs interval case
```suggestion
test("scalar string and numeric types coerce, matching the comparison
operator") {
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2749,7 +2728,62 @@ object AsOfJoin {
def isValidOperandType(dataType: DataType): Boolean =
RowOrdering.isOrderable(dataType) && !containsEmptyStructType(dataType)
+ /**
+ * Top-level operand compatibility. A string vs non-string scalar pair is
compatible only when
+ * the comparison operator has a common type for it, so it coerces like
`>=` (accepting DATE vs
+ * STRING, rejecting STRING vs INTERVAL). Other scalars widen;
STRUCT/ARRAY use the stricter
+ * [[areFieldTypesCompatible]] rule.
+ */
def areOperandsCompatible(leftType: DataType, rightType: DataType):
Boolean = {
+ if (!isValidOperandType(leftType) || !isValidOperandType(rightType)) {
+ false
+ } else if (isCompositeOperand(leftType, rightType)) {
+ areFieldTypesCompatible(leftType, rightType)
+ } else if (isExactlyOneStringPair(leftType, rightType)) {
+ matchComparisonCommonType(leftType, rightType).isDefined
+ } else {
+ TypeCoercion.findWiderTypeForTwo(leftType, rightType).isDefined
+ }
+ }
+
+ /**
+ * The type a string vs non-string scalar pair is cast to for comparison,
ordering, and sort,
+ * mirroring the comparison operator's string coercion in the active mode
(ANSI or default).
+ * Returns [[None]] for other pairs, whose monotonic widening keeps the
raw operands.
+ */
+ private[catalyst] def matchComparisonCommonType(
Review Comment:
It's not really clear from the naming that this is only for cases when one
of the types is string, `stringComparisonCommonType` or
`comparisonCommonTypeForString` might read better
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/logical/AsOfJoinMatchConditionTypesSuite.scala:
##########
@@ -30,18 +29,43 @@ class AsOfJoinMatchConditionTypesSuite extends
SparkFunSuite {
assert(!MatchConditionTypes.usesStructDecomposition(IntegerType, LongType))
}
- test("string and temporal types are incompatible") {
- assert(!MatchConditionTypes.areOperandsCompatible(StringType,
TimestampType))
- assert(!MatchConditionTypes.areOperandsCompatible(DateType, StringType))
+ test("scalar string and temporal types coerce like the comparison operator")
{
+ assert(MatchConditionTypes.areOperandsCompatible(StringType,
TimestampType))
+ assert(MatchConditionTypes.areOperandsCompatible(DateType, StringType))
+ // Common type is the temporal type (string cast to it), so sort and
comparison agree.
Review Comment:
```suggestion
// Common type is the temporal type (string is cast to it), so sort and
comparison agree.
```
--
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]
