uros-b commented on code in PR #58609:
URL: https://github.com/apache/spark/pull/58609#discussion_r3959775584
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/logical/AsOfJoinMatchConditionTypesSuite.scala:
##########
@@ -117,4 +117,68 @@ class AsOfJoinMatchConditionTypesSuite extends
SparkFunSuite {
assert(!MatchConditionTypes.usesStructDecomposition(leftStruct,
rightStruct))
assert(!MatchConditionTypes.areOperandsCompatible(leftStruct, rightStruct))
}
+
+ test("map operands are invalid (not orderable)") {
+ val mapType = MapType(StringType, IntegerType)
+ assert(!MatchConditionTypes.isValidOperandType(mapType))
+ assert(!MatchConditionTypes.areOperandsCompatible(mapType, mapType))
+ }
+
+ test("array operands with non-orderable elements are invalid") {
+ val arrayOfMap = ArrayType(MapType(StringType, IntegerType))
+ assert(!MatchConditionTypes.isValidOperandType(arrayOfMap))
+ assert(!MatchConditionTypes.areOperandsCompatible(arrayOfMap, arrayOfMap))
+ assert(!MatchConditionTypes.usesArrayOrderExpression(arrayOfMap,
arrayOfMap))
+ }
+
+ test("struct operands with a non-orderable field are invalid") {
+ val structWithMap = StructType(
+ StructField("a", IntegerType) ::
+ StructField("m", MapType(StringType, IntegerType)) ::
+ Nil)
+ assert(!MatchConditionTypes.isValidOperandType(structWithMap))
+ assert(!MatchConditionTypes.areOperandsCompatible(structWithMap,
structWithMap))
+ }
+
+ test("positional struct operands with incompatible field types are
rejected") {
+ val leftStruct = StructType(
+ StructField("a", IntegerType) ::
+ StructField("b", TimestampType) ::
+ Nil)
+ val rightStruct = StructType(
+ StructField("x", IntegerType) ::
+ StructField("y", BooleanType) ::
+ Nil)
+ // Same field count, so the operands are structurally decomposable ...
+ assert(MatchConditionTypes.usesStructDecomposition(leftStruct,
rightStruct))
+ // ... but the second field pair (TIMESTAMP vs BOOLEAN) is not comparable.
+ assert(!MatchConditionTypes.areOperandsCompatible(leftStruct, rightStruct))
+ }
+
+ test("array operands with empty struct elements are invalid") {
+ // An array whose element type contains an empty struct is not a valid
operand, even though
+ // the array itself is orderable (exercises the ArrayType arm of
containsEmptyStructType).
+ val arrayOfEmptyStruct = ArrayType(StructType(Nil))
+ assert(!MatchConditionTypes.isValidOperandType(arrayOfEmptyStruct))
+ assert(!MatchConditionTypes.areOperandsCompatible(arrayOfEmptyStruct,
arrayOfEmptyStruct))
+ }
+
+ test("array operands with structurally incompatible struct elements are
rejected") {
Review Comment:
This is a bit misleading test name. array operands with structurally
incompatible struct elements are rejected is the same situation as positional
struct operands with incompatible field types: same field count (decomposable),
incompatible field types. “Structurally incompatible” reads like the existing
field-count case (usesStructDecomposition == false). Prefer something like
`array operands with incompatible struct element field types are rejected`.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/logical/AsOfJoinMatchConditionTypesSuite.scala:
##########
@@ -117,4 +117,68 @@ class AsOfJoinMatchConditionTypesSuite extends
SparkFunSuite {
assert(!MatchConditionTypes.usesStructDecomposition(leftStruct,
rightStruct))
assert(!MatchConditionTypes.areOperandsCompatible(leftStruct, rightStruct))
}
+
+ test("map operands are invalid (not orderable)") {
+ val mapType = MapType(StringType, IntegerType)
+ assert(!MatchConditionTypes.isValidOperandType(mapType))
+ assert(!MatchConditionTypes.areOperandsCompatible(mapType, mapType))
+ }
+
+ test("array operands with non-orderable elements are invalid") {
+ val arrayOfMap = ArrayType(MapType(StringType, IntegerType))
+ assert(!MatchConditionTypes.isValidOperandType(arrayOfMap))
+ assert(!MatchConditionTypes.areOperandsCompatible(arrayOfMap, arrayOfMap))
+ assert(!MatchConditionTypes.usesArrayOrderExpression(arrayOfMap,
arrayOfMap))
+ }
+
+ test("struct operands with a non-orderable field are invalid") {
+ val structWithMap = StructType(
+ StructField("a", IntegerType) ::
+ StructField("m", MapType(StringType, IntegerType)) ::
+ Nil)
+ assert(!MatchConditionTypes.isValidOperandType(structWithMap))
+ assert(!MatchConditionTypes.areOperandsCompatible(structWithMap,
structWithMap))
+ }
+
+ test("positional struct operands with incompatible field types are
rejected") {
+ val leftStruct = StructType(
+ StructField("a", IntegerType) ::
+ StructField("b", TimestampType) ::
+ Nil)
+ val rightStruct = StructType(
+ StructField("x", IntegerType) ::
+ StructField("y", BooleanType) ::
+ Nil)
+ // Same field count, so the operands are structurally decomposable ...
+ assert(MatchConditionTypes.usesStructDecomposition(leftStruct,
rightStruct))
+ // ... but the second field pair (TIMESTAMP vs BOOLEAN) is not comparable.
+ assert(!MatchConditionTypes.areOperandsCompatible(leftStruct, rightStruct))
+ }
+
+ test("array operands with empty struct elements are invalid") {
Review Comment:
The empty struct array test does not isolate the path it claims. The comment
says the array is orderable and that rejection comes from
containsEmptyStructType. That is true (StructType.fields.forall is true for
Nil, and arrays recurse into the element type), but the test only checks the
combined isValidOperandType predicate. If empty structs later became
non-orderable, this test would still pass and would no longer cover the
ArrayType arm. Please consider pinning it, e.g.:
```
assert(RowOrdering.isOrderable(arrayOfEmptyStruct))
assert(!MatchConditionTypes.isValidOperandType(arrayOfEmptyStruct))
```
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/logical/AsOfJoinMatchConditionTypesSuite.scala:
##########
@@ -117,4 +117,68 @@ class AsOfJoinMatchConditionTypesSuite extends
SparkFunSuite {
assert(!MatchConditionTypes.usesStructDecomposition(leftStruct,
rightStruct))
assert(!MatchConditionTypes.areOperandsCompatible(leftStruct, rightStruct))
}
+
+ test("map operands are invalid (not orderable)") {
+ val mapType = MapType(StringType, IntegerType)
+ assert(!MatchConditionTypes.isValidOperandType(mapType))
+ assert(!MatchConditionTypes.areOperandsCompatible(mapType, mapType))
+ }
+
+ test("array operands with non-orderable elements are invalid") {
+ val arrayOfMap = ArrayType(MapType(StringType, IntegerType))
+ assert(!MatchConditionTypes.isValidOperandType(arrayOfMap))
+ assert(!MatchConditionTypes.areOperandsCompatible(arrayOfMap, arrayOfMap))
+ assert(!MatchConditionTypes.usesArrayOrderExpression(arrayOfMap,
arrayOfMap))
+ }
+
+ test("struct operands with a non-orderable field are invalid") {
+ val structWithMap = StructType(
+ StructField("a", IntegerType) ::
+ StructField("m", MapType(StringType, IntegerType)) ::
+ Nil)
+ assert(!MatchConditionTypes.isValidOperandType(structWithMap))
+ assert(!MatchConditionTypes.areOperandsCompatible(structWithMap,
structWithMap))
+ }
+
+ test("positional struct operands with incompatible field types are
rejected") {
+ val leftStruct = StructType(
+ StructField("a", IntegerType) ::
+ StructField("b", TimestampType) ::
+ Nil)
+ val rightStruct = StructType(
+ StructField("x", IntegerType) ::
+ StructField("y", BooleanType) ::
+ Nil)
+ // Same field count, so the operands are structurally decomposable ...
+ assert(MatchConditionTypes.usesStructDecomposition(leftStruct,
rightStruct))
+ // ... but the second field pair (TIMESTAMP vs BOOLEAN) is not comparable.
+ assert(!MatchConditionTypes.areOperandsCompatible(leftStruct, rightStruct))
+ }
+
+ test("array operands with empty struct elements are invalid") {
+ // An array whose element type contains an empty struct is not a valid
operand, even though
+ // the array itself is orderable (exercises the ArrayType arm of
containsEmptyStructType).
+ val arrayOfEmptyStruct = ArrayType(StructType(Nil))
+ assert(!MatchConditionTypes.isValidOperandType(arrayOfEmptyStruct))
+ assert(!MatchConditionTypes.areOperandsCompatible(arrayOfEmptyStruct,
arrayOfEmptyStruct))
+ }
+
+ test("array operands with structurally incompatible struct elements are
rejected") {
+ val leftArray = ArrayType(
+ StructType(
+ StructField("a", IntegerType) ::
+ StructField("b", TimestampType) ::
+ Nil))
+ val rightArray = ArrayType(
+ StructType(
+ StructField("x", IntegerType) ::
+ StructField("y", BooleanType) ::
+ Nil))
+ // Each operand is individually a valid, orderable type ...
+ assert(MatchConditionTypes.isValidOperandType(leftArray))
+ assert(MatchConditionTypes.isValidOperandType(rightArray))
+ // ... but the element structs' second field pair (TIMESTAMP vs BOOLEAN)
is not comparable.
+ assert(!MatchConditionTypes.areOperandsCompatible(leftArray, rightArray))
+ assert(!MatchConditionTypes.usesArrayOrderExpression(leftArray,
rightArray))
Review Comment:
Do not assert !usesArrayOrderExpression on ARRAY<STRUCT<>>.
areArrayElementsCompatible treats two empty structs as sameType and then asks
isOrderable, which is true. So usesArrayOrderExpression is true even though
areOperandsCompatible is false (validity short-circuit).
--
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]
