Visorgood commented on issue #5971: URL: https://github.com/apache/datafusion-comet/issues/5971#issuecomment-5774043318
Thanks @andygrove! All four points addressed: 1. **Celeborn**. Confirmed, and the guard is stronger than dependency-level: `getWriter` reaches the partitioning only through `nativeDependency`, which requires `shuffleType == CometNativeShuffle`, and the `None` branch calls `rejectCometHandle`, which throws for both columnar handles. Comet columnar shuffle over Celeborn is unsupported outright. Added to the chain. 2. **Hash branch**. Agreed, taken in the same PR. Same argument, same conclusion – the columnar path evaluates `h.partitionIdExpression` through `UnsafeProjection` and never serializes it. Pinned with a failing-then-passing test. Removing it did surface one thing: `columnar shuffle on array/struct map key/value` expected 0 exchanges on 4.0+ because Spark wraps map keys in `mapsort(...)`. Since the JVM computes partition ids including `mapsort`, that fallback was also unnecessary; expectation is now 1, with a `spark_partition_id()` test pinning that the distribution still matches Spark. 3. **Collation**. Neither half of your dichotomy, I think. I wrote probes for collated hash and range keys and both fell back before shuffle was consulted – `CometScanRule` rejects collated columns at the scan, with the comment "_we don't need specific support for collation in scans, but this is a convenient place to force the whole query to fall back to Spark for now_". So no collated data reaches columnar shuffle, the round trip never runs, and there's nothing to measure. The payload-vs-key inconsistency you spotted is real but unobservable for the same reason. I've left the checks. They're unreachable rather than dead, and they become meaningful the day that scan-level fallback goes away – at which point the Arrow round-trip question you raised is a real one that deserves its own answer. Happy to remove them if you'd rather, but I couldn't justify it with a test. 4. **Default-config test**. You're right that the struct-with-double case needs an opt-in. `repartitionByRange($"a" + 1)` won't do it either – `Add` has a serde, so the probe passes. I used a Scala UDF with `spark.comet.exec.scalaUDF.codegen.enabled=false`, where `CometScalaUDF.convert` returns `None` by config rather than by absence of support – so the test stays meaningful even if serde coverage grows. One for hash, one for range. 100 tests green across both AQE configurations; a full `mvnw test` passed 3502 with no failures. -- 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]
