Double-negated child check and an always-true guard in the shuffle support logic #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