comphead commented on code in PR #6533:
URL: https://github.com/apache/datafusion-comet/pull/6533#discussion_r4417473218
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -334,6 +334,14 @@ object CometArrayExcept
private val incompatReason = "Null handling and ordering may differ from
Spark"
+ // Helper function to recursively make an array type fully nullable
+ // This ensures nested arrays are also made nullable at all levels
+ private def makeRecursivelyNullable(dt: DataType): DataType = dt match {
+ case ArrayType(elementType, _) =>
+ ArrayType(makeRecursivelyNullable(elementType), containsNull = true)
+ case other => other
Review Comment:
`DataTypeSupport.deepNullable` already does this, and `arrays.scala` imports
it at line 32. It also widens map values and struct fields, which this helper
leaves alone. Could the helper go and both operands use `deepNullable`? `Cast`
is imported too, so lines 402 and 408 do not need the fully qualified name.
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -376,15 +384,40 @@ object CometArrayExcept
return None
case None =>
}
- val leftArrayExprProto = exprToProtoInternal(expr.left, inputs, binding)
- val rightArrayExprProto = exprToProtoInternal(expr.right, inputs, binding)
+
+ // Fix for issue #3646: Normalize element type nullability recursively to
avoid DataFusion
+ // type mismatch. DataFusion's array_except requires both arrays to have
the same element
+ // nullability at all levels (including nested arrays).
+ //
Review Comment:
Lines 391 and 397 end in trailing spaces, which `WhitespaceEndOfLineChecker`
in `scalastyle-config.xml` reports as an error, so the lint job will fail once
CI runs. `make format` or removing the spaces fixes it.
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -376,15 +384,40 @@ object CometArrayExcept
return None
case None =>
}
- val leftArrayExprProto = exprToProtoInternal(expr.left, inputs, binding)
- val rightArrayExprProto = exprToProtoInternal(expr.right, inputs, binding)
+
+ // Fix for issue #3646: Normalize element type nullability recursively to
avoid DataFusion
+ // type mismatch. DataFusion's array_except requires both arrays to have
the same element
+ // nullability at all levels (including nested arrays).
+ //
+ // We recursively make both types fully nullable to match the existing
widen-nullability
+ // behavior of CometCreateArray. This ensures compatibility with
DataFusion's type system
+ // while preserving the ability to handle nested arrays.
+ val leftType = expr.left.dataType
+ val rightType = expr.right.dataType
+
+ val normalizedLeftType = makeRecursivelyNullable(leftType)
+ val normalizedRightType = makeRecursivelyNullable(rightType)
+
+ val normalizedLeft = if (leftType != normalizedLeftType) {
+ org.apache.spark.sql.catalyst.expressions.Cast(expr.left,
normalizedLeftType)
+ } else {
+ expr.left
+ }
+
+ val normalizedRight = if (rightType != normalizedRightType) {
+ org.apache.spark.sql.catalyst.expressions.Cast(expr.right,
normalizedRightType)
+ } else {
+ expr.right
+ }
+
+ val leftArrayExprProto = exprToProtoInternal(normalizedLeft, inputs,
binding)
+ val rightArrayExprProto = exprToProtoInternal(normalizedRight, inputs,
binding)
Review Comment:
The PR has no regression test yet. Could
`sql-tests/expressions/array/array_except.sql`, or a sibling file with `--
Config: spark.comet.expression.ArrayExcept.allowIncompatible=true`, add the
query from #3646 and sunchao's nested case `array_except(array(array(id)),
array(IF(id = 0, array(2L), NULL)))` in both operand orders? Without the config
line the query takes the dispatcher path on main and would not exercise this
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: [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]
