viirya opened a new issue, #5563:
URL: https://github.com/apache/datafusion-comet/issues/5563
## Is your feature request related to a problem or challenge?
`CometShuffleExchangeExec.supportedSerializableDataType` (the native shuffle
data-column type gate) allows struct data columns, recursing through
`StructType` / `ArrayType` / `MapType`:
```scala
case StructType(fields) =>
fields.nonEmpty && fields.forall(f =>
supportedSerializableDataType(f.dataType))
case ArrayType(elementType, _) =>
supportedSerializableDataType(elementType)
case MapType(keyType, valueType, _) =>
supportedSerializableDataType(keyType) &&
supportedSerializableDataType(valueType)
```
But `CometNativeShuffleSuite` never exercised a struct data column. The only
nested coverage in that suite is:
- `native shuffle on nested array` -- `array<array<int>>`
- `native shuffle with Map[_, NullType] column` -- `map<int, null>`
The word `struct` does not appear anywhere in the suite. By contrast,
`CometColumnarShuffleSuite` has extensive nested coverage (struct with nulls,
array/struct as map key/value, `array<map>`, many map value types), so the
gap
is specific to native shuffle.
This matters because nested types in shuffle have been a real source of bugs
--
e.g. cd4d0e25d ("fix: normalize nested field nullability in ShuffleScanExec
and
ExpandExec", #5137), where Arrow treats nested field nullability as part of
`DataType` identity and the decoded shuffle block had to be reconciled
against
the catalyst-declared schema. A behavior the gate explicitly claims to
support,
with no end-to-end test, can regress silently.
Note that struct columns can only reach native shuffle as *data* columns:
`supportedHashPartitioningDataType` rejects nested types as a hash key, so a
query partitioning on a struct falls back to Spark. Tests therefore need to
partition on a primitive and carry the struct along.
## Describe the solution you'd like
Add native shuffle tests covering struct data columns, including nulls, the
recursive nesting branches of the type gate (`struct<array<...>>`,
`array<struct<...>>`), and a struct containing a map field.
## Describe alternatives you've considered
None -- this is purely additional test coverage for existing,
already-supported
behavior. No product code change is proposed.
## Additional context
Verified by mutation testing: temporarily changing the native gate's
`case StructType(...)` to `false` makes exactly the new tests fail and leaves
the other 36 tests in the suite passing, confirming they assert native struct
shuffle rather than passing vacuously via a silent fallback to Spark.
--
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]
