0lai0 opened a new pull request, #7045: URL: https://github.com/apache/datafusion-comet/pull/7045
## Which issue does this PR close? Closes #7005 ## Rationale for this change The shuffle support logic in `CometShuffleExchangeExec` had three readability issues that the issue asked to clean up. This code decides between native, JVM, and Spark shuffle and changes frequently, so clearer logic lowers the risk of a future mistake. Every rewrite is a pure logical equivalence, so behavior is unchanged. ## What changes are included in this PR? In `object CometShuffleExchangeExec`: - `columnarShuffleFailureReasons`: replaced the double-negated child check `!(!s.child.supportsColumnar || isCometPlan(s.child))` with its De Morgan equivalent `s.child.supportsColumnar && !isCometPlan(s.child)`. Both forms evaluate `supportsColumnar` first and short-circuit on the same value. - `shuffleSupported`: removed the always-true `if (combined.nonEmpty)` guard. Execution only reaches that point when `columnarReasons` is non-empty, so the tail is now `withFallbackReasons(s, (nativeReasons ++ columnarReasons).toSet); None`. - `isCometNativeShuffleMode` / `isCometJVMShuffleMode`: merged the separate `true` arms into `case "native" | "auto" => true` and `case "jvm" | "auto" => true`. `COMET_SHUFFLE_MODE` is restricted to `native`, `jvm`, and `auto` by `checkValues`. In `CometColumnarShuffleSuite`, added one test for the child check, because no existing test reached it. ## How are these changes tested? Added `"Fallback to Spark when shuffling a columnar child that is not a Comet operator"` to `CometColumnarShuffleSuite`. With the Comet scan disabled, the shuffle's child is Spark's columnar Parquet scan, so the columnar shuffle must fall back to Spark. No existing test reached this branch. With the check removed, every existing test still passed, but the new test fails with a `ClassCastException` (`ColumnarBatch` cast to `InternalRow`). The new test passes on both the original and the rewritten condition. Ran locally on Spark 4.1: - `CometShuffleSuite`, `DisableAQECometShuffleSuite`, `CometShuffle4_0Suite`, `CometNativeShuffleSuite`, `CometShuffleInputConversionSuite`: all passed. - `spotless:check` and `scalastyle:check`: pass. - The new test also passes on `-Pspark-3.4`. Not covered: no test reaches the `withFallbackReasons` call at the end of `shuffleSupported`. Its equivalence rests on the reachability argument above. ### Follow-ups - The fallback reason "is a neither row-based or a Comet operator" is ungrammatical. It is user-visible, so I left it for a separate PR. - A test that pins which `ShuffleType` `shuffleSupported` returns for each mode and child type would make future changes here safer. -- 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]
