andygrove opened a new issue, #7005:
URL: https://github.com/apache/datafusion-comet/issues/7005
### What is the problem the feature request solves?
`columnarShuffleFailureReasons` tests `!(!s.child.supportsColumnar ||
isCometPlan(s.child))`, a double negation that is hard to read. In
`shuffleSupported`, `if (combined.nonEmpty)` is always true, because
`columnarReasons` was just found to be non-empty. Separately,
`isCometNativeShuffleMode` and `isCometJVMShuffleMode` are 6-line pattern
matches that each test whether a string is one of two values. This code decides
native vs. JVM vs. Spark shuffle and changes frequently.
Suggested fix:
Apply De Morgan to get `if (s.child.supportsColumnar &&
!isCometPlan(s.child))`. Replace the tail of shuffleSupported with
`withFallbackReasons(s, (nativeReasons ++ columnarReasons).toSet); None`, since
columnarReasons is non-empty at that point. Write the mode helpers as
`COMET_SHUFFLE_MODE.get(conf) match { case "native" | "auto" => true; case _ =>
false }` (and `"jvm" | "auto"`). Every rewrite is a pure logical equivalence,
so behavior is unchanged.
Code context:
```
val columnarReasons = columnarShuffleFailureReasons(s)
if (columnarReasons.isEmpty) {
return Some(CometColumnarShuffle)
}
val combined = (nativeReasons ++ columnarReasons).toSet
if (combined.nonEmpty) withFallbackReasons(s, combined)
None
...
if (!(!s.child.supportsColumnar || isCometPlan(s.child))) {
reasons += s"Child ${s.child.getClass.getName} is a neither row-based
or a Comet operator"
...
private def isCometNativeShuffleMode(conf: SQLConf): Boolean = {
COMET_SHUFFLE_MODE.get(conf) match {
case "native" => true
case "auto" => true
case _ => false
}
}
```
### Describe the potential solution
_No response_
### Additional context
_No response_
### Willingness to contribute
None
--
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]
