andygrove commented on issue #5971: URL: https://github.com/apache/datafusion-comet/issues/5971#issuecomment-5765674640
Thanks for verifying, @Visorgood. I walked the same chain and agree the probe is dead on the columnar path. A few additions before you open the PR. One gap in the verification: `CometNativeShuffleHandle` isn't the only reader of the partitioning. `CometCelebornShuffleManager.getWriter` also pulls `dependency.outputPartitioning` and `dependency.rangePartitionBounds` (around line 171). It reaches the same conclusion — it's guarded on `shuffleType == CometNativeShuffle`, and `rejectCometHandle` bails out for both `CometBypassMergeSortShuffleHandle` and `CometSerializedShuffleHandle` — but since the ask in the description was to prove that *no* columnar write path consumes a serialized partitioning, that second manager should be part of the answer. The bigger thing is that the range branch isn't the only dead probe in `columnarShuffleFailureReasons`. The `HashPartitioning` branch just above it runs the same `exprToProto` check, and the columnar path evaluates `h.partitionIdExpression` through `UnsafeProjection` on the JVM exactly the way it does the range keys — never serializing either. Same argument, same conclusion. Would you take both in one PR? Fixing only range leaves the identical bug one case up. On keeping the collation check, I'm not sure that rationale survives. `LazilyGeneratedOrdering` is collation-aware, and on the hash side Spark's `Murmur3Hash` hashes the collation key, so JVM partitioning is correct for collated strings either way. `supportedSerializableDataType` also already accepts collated strings as data columns, so as things stand we reject a type as a partition key that we happily ship as payload. Either that check is dead too, or collation isn't surviving the Arrow round trip and we have a worse bug in the data path. I haven't checked the round trip — could you work out which before deciding to keep it? Minor last point: `spark.comet.exec.strictFloatingPoint` defaults to false, so the struct-with-double repro needs an explicit opt-in. That's fine for a test, but the case that bites at default config is the second bullet in the description, an ordering expression with no serde at all — `repartitionByRange($"a" + 1)` and `ORDER BY f(a)` both put arbitrary expressions in `RangePartitioning`. Worth covering that shape 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]
