andygrove commented on code in PR #6671:
URL: https://github.com/apache/datafusion-comet/pull/6671#discussion_r4283390760
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -270,6 +273,9 @@ object CometCast
case (DataTypes.DoubleType, _) =>
canCastFromDouble(toType)
case (from_struct: StructType, to_struct: StructType) =>
+ if (hasDuplicateFieldNames(to_struct.fields)) {
+ return Unsupported(Some(duplicateStructFieldNamesReason))
+ }
Review Comment:
This check only runs when the source is a struct too. `isSupported` returns
`Compatible()` for a `NullType` source and for an `ARRAY<NULL>` source before
reaching it, and a struct field whose source is `NULL` takes the `NullType`
branch on the way down. So these still crash with `ArrowArray struct has 2
children (expected 1)` on this branch, with default configs:
```sql
SELECT CAST(named_struct('a', NULL, 'b', b) AS STRUCT<a: STRUCT<x: INT, x:
INT>, b: INT>) FROM t
SELECT a, CAST(e AS STRUCT<x: INT, x: INT>) FROM t LATERAL VIEW
explode(array(NULL, NULL)) v AS e
```
Could we check the whole target type once near the top of `isSupported`
instead? It would walk struct fields, array elements, and map keys and values
the same way `hasCaseInsensitiveDuplicateFieldNames` does, and could live next
to it in `DataTypeSupport`:
```scala
if (containsDuplicateFieldNames(toType)) {
return Unsupported(Some(duplicateStructFieldNamesReason))
}
```
I tried that locally together with the `CometLiteral` change from the review
body. Both queries then match Spark, and `CometNativeCastSuite`,
`CometLiteralSuite` and the cast, struct, array, map and conditional fixtures
still pass. Could you add these queries to the fixture too?
--
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]
