vranes commented on code in PR #58827:
URL: https://github.com/apache/spark/pull/58827#discussion_r4047651597
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -17,6 +17,7 @@
Review Comment:
1. PR description doesn't describe the array < struct > behaviour before vs
now. Its behaviour is changed in a different way than arrays with basic element
types, it should be properly documented.
2. We should fix the bare structs in the same way (ASOF validation accepts
different field names with different types, but it fails in a later validation
stage).
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2768,16 +2769,27 @@ object AsOfJoin {
def usesArrayOrderExpression(leftType: DataType, rightType: DataType):
Boolean =
(leftType, rightType) match {
case (ArrayType(leftElem, _), ArrayType(rightElem, _)) =>
- areArrayElementsCompatible(leftElem, rightElem)
+ // MATCH_CONDITION compares the two arrays with `>=`, which widens
array elements only
+ // through findTightestCommonType (no string promotion, no decimal
widening). Accept
+ // exactly what that comparison can compare: orderable elements that
are already
+ // structurally equal (BinaryComparison ignores struct field names
and nullability) or
+ // have a tightest common type. Otherwise the type check would pass
but the `>=` would
+ // fail to resolve.
+ isValidOperandType(leftElem) && isValidOperandType(rightElem) &&
+ (DataType.equalsStructurally(leftElem, rightElem,
ignoreNullability = true) ||
+ arrayElementCommonType(leftElem, rightElem).isDefined)
case _ => false
}
- private def areArrayElementsCompatible(leftElem: DataType, rightElem:
DataType): Boolean = {
- if (DataTypeUtils.sameType(leftElem, rightElem)) {
- RowOrdering.isOrderable(leftElem)
- } else {
- arePositionalStructsCompatible(leftElem, rightElem)
- }
+ /**
+ * The element type the `>=` comparison coerces two array operands to, if
any. Binary
Review Comment:
This reads as if this is only about that specific comparison operator (>=).
Replace with "binary comparison" and simplify the comment. Perhaps add an
example also to make it clear.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2988,8 +3000,30 @@ object AsOfJoin {
private def buildArrayOrderExpression(
leftOperand: Expression,
rightOperand: Expression,
- elementType: DataType,
operator: MatchComparisonOperator): Expression = {
+ val leftElementType =
leftOperand.dataType.asInstanceOf[ArrayType].elementType
+ val rightElementType =
rightOperand.dataType.asInstanceOf[ArrayType].elementType
+ // The ZipWith lambda variables and both array inputs must share the
element type the `>=`
+ // comparison coerces to. Coercible elements (e.g. INT vs BIGINT, or INT
vs FLOAT which widens
+ // to DOUBLE under ANSI) widen to their tightest common type and both
arrays are cast to it.
+ // Structurally equal elements (BinaryComparison ignores struct field
names) need no cast and
+ // compare element-wise by ordinal.
+ val elementsStructurallyEqual =
+ DataType.equalsStructurally(leftElementType, rightElementType,
ignoreNullability = true)
+ val (leftArray, rightArray, elementType) =
+ if (elementsStructurallyEqual) {
+ (leftOperand, rightOperand, leftElementType)
+ } else {
+ MatchConditionTypes.arrayElementCommonType(leftElementType,
rightElementType) match {
+ case Some(widerElementType) =>
+ (castArrayElementType(leftOperand, widerElementType),
+ castArrayElementType(rightOperand, widerElementType),
+ widerElementType)
+ case None =>
+ // Unreachable: usesArrayOrderExpression already required a common
element type here.
+ throw SparkException.internalError("MATCH_CONDITION array elements
have no common type")
Review Comment:
`usesArrayOrderExpression` and `buildArrayOrderExpression` both decide the
array element type the same way. Consider a shared `MatchConditionTypes` helper
returning `Option[DataType]` so the two can't drift.
--
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]
