Visorgood commented on code in PR #6110:
URL: https://github.com/apache/datafusion-comet/pull/6110#discussion_r4079850225
##########
spark/src/main/scala/org/apache/spark/sql/comet/execution/shuffle/CometShuffleExchangeExec.scala:
##########
@@ -637,11 +637,6 @@ object CometShuffleExchangeExec
val partitioning = s.outputPartitioning
partitioning match {
case HashPartitioning(expressions, _) =>
- for (expr <- expressions) {
- if (QueryPlanSerde.exprToProto(expr, inputs).isEmpty) {
- reasons += s"unsupported hash partitioning expression: $expr"
- }
- }
for (dt <- expressions.map(_.dataType).distinct) {
if (isStringCollationType(dt)) {
Review Comment:
You're right that nothing native sees the key: the partition id comes from
Spark's Murmur3Hash / LazilyGeneratedOrdering on the JVM, and both are
collation-aware.
But I believe the checks still can't go. Rejecting the exchange is what
moves the whole stage off Comet, and that also keeps CometSort off the collated
key. supportedSortType only type-checks single-column sorts
(QueryPlanSerde.scala:1287), so a multi-column collated sort gets past it.
I removed both checks and ran CometCollationSuite: 4 failures. Three only
change the reason string the test pins, since the query still falls back via
the sort check. The fourth is a wrong answer:
- listagg DISTINCT with utf8_lcase collation (issue #1947) *** FAILED ***
!== Spark Answer - 1 == == Comet Answer - 1 ==
![ab] [aabb]
The plan keeps CometColumnarExchange and CometSort over a two-column
collated sort key, so Comet dedups a/A on raw bytes and #1947 is back.
Kept them, and rewrote the rationale in the description – my "unreachable"
claim there was wrong. Also added a test for the fallback, which the suite
didn't have. I'll file a separate issue for the single-column limit in
supportedSortType; fixing that is what would make this check redundant.
--
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]
