Visorgood commented on code in PR #6110:
URL: https://github.com/apache/datafusion-comet/pull/6110#discussion_r4126664372
##########
docs/source/contributor-guide/jvm_shuffle.md:
##########
@@ -54,6 +54,20 @@ JVM shuffle (`CometColumnarExchange`) is used instead of
native shuffle (`CometE
[Supported partition key
types](native_shuffle.md#when-native-shuffle-is-used) for the exact
rules. Complex types are fully supported as data columns in both
implementations.
+4. **Partition keys native shuffle cannot serialize**: native shuffle
serializes the
+ partitioning expressions to protobuf, so a key expression Comet has no
serde for, or whose
+ serde reports it incompatible, keeps the exchange off the native path. JVM
shuffle has no
+ such requirement, because it evaluates the key on the JVM through
`UnsafeProjection` and
+ `LazilyGeneratedOrdering`, so these exchanges land here rather than on
Spark's shuffle. One
+ example is the `mapsort(...)` wrapper Spark 4.0 and later adds around a map
used as a
+ shuffle key: Comet cannot serialize it for array or struct map keys, so
such an exchange
+ becomes `CometColumnarExchange`.
Review Comment:
Good catch. Added item 5 to "When Native Shuffle is Used": native shuffle
serializes the partitioning into its protobuf plan, so every HashPartitioning
expression and every RangePartitioning sort order has to convert through
QueryPlanSerde.exprToProto.
For the mapsort example I named CometMapSort explicitly, since item 4
mentions mapsort as what *admits* map keys and the two could otherwise look
contradictory: the type is admissible with nested hash keys enabled, but
CometMapSort supports scalar map keys only, so a map with array or struct keys
fails the expression check. That matches the comment already in
supportedHashPartitioningDataType. Cross-linked to jvm_shuffle.md so the two
lists agree.
The workflows will need approving again.
--
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]
