apache / apache/datafusion-comet
Tag fallback reasons with structured values instead of pre-formatted strings
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
## What is the problem the feature request solves?
Fallback reasons are currently recorded as pre-formatted strings. `withFallbackReason` / `withFallbackReasons` accumulate a `Set[String]` on the `CometExplainInfo.FALLBACK_REASONS` tag, so every call site decides the final user-facing wording at tag time and the only thing downstream consumers (extended explain, the fallback log, tests) can do with a reason is print it or substring-match it.
There are ~205 `withFallbackReason` call sites today, and the string representation is causing several concrete problems:
**1. Tests substring-match on message wording.** `CometTestBase.checkSparkAnswerAndFallbackReasons` asserts with `actualFallbacks.exists(_.contains(reason))`. This makes the exact phrasing of every message a de-facto public API — rewording one message breaks tests that have nothing to do with the change — while simultaneously being loose enough to pass accidentally on an unrelated reason that happens to contain the substring.
**2. Deduplication happens on rendered text, not on the reason.** The tag is a `Set[String]` and most messages interpolate expression or data type text, so two occurrences of the same underlying reason that render slightly differently become two distinct set members and are both reported.
**3. Phrasing has already drifted.** At least five spellings of a single reason exist in the codebase:
```
Unsupported data type
Unsupported data type: ${expr.dataType}
Unsupported datatype $dataType
Unsupported datatype in castToProto: $dt
Failed to serialize datatype ${expr.dataType} for scalar subquery
```
The same is true of the "a config is disabled" family (`X is not enabled`, `requires X to be enabled`, `Set X`, `Expression support is disabled. Set ...`), where some variants include the "how to fix it" hint and some do not. `NativeOptIn.message` and `SupportLevel.strictFloatingPointReason` already exist specifically to hand-roll shared wording for two of these families, which suggests the general mechanism is missing.
**4. A newline-splitting hack exists only because reasons are strings.** `withFallbackReason` splits its argument on `"\n"` to support call sites that pack several reasons into one string.
**5. Reasons are not queryable.** The only programmatic signal available is `hasFallbackReason`, a boolean. Nothing can distinguish "fell back because a config is disabled" from "fell back because of a genuine gap in Comet", which is information wanted by `spark.comet.strictFallbackReasons`, by coverage reporting, and by anything that wants to aggregate the top real gaps across a benchmark suite without regexing explain output.
## Describe the potential solution
Tag with a raw, structured reason and move formatting to the point of consumption (extended explain and the fallback log). A sealed hierarchy mirroring the existing `SupportLevel` pattern:
```scala
sealed trait FallbackReason { def message: String } // default rendering
object FallbackReason {
case class ConfigDisabled(conf: ConfigEntry[_]) extends FallbackReason
case class UnsupportedDataType(dt: DataType, context: Option[String] = None) extends FallbackReason
case class UnsupportedExpression(name: String, notes: Option[String] = None) extends FallbackReason
case class UnsupportedOperator(name: String) extends FallbackReason
case class NotCompatible(name: String, notes: Option[String], enableConf: ConfigEntry[_]) extends FallbackReason
case class Other(message: String) extends FallbackReason // escape hatch
}
```
with the tag becoming `TreeNodeTag[Set[FallbackReason]]`.
Two design notes:
- Keep a `message` on the trait rather than pushing all rendering into `ExtendedExplainInfo`. The fallback-log path in `withFallbackReasons` needs a rendering too, and co-locating the default wording with the case keeps it discoverable. Explain can still layer its own decoration on top (grouping, truncation, verbose vs. fallback format, appending the config key and compatibility-guide link).
- Store data types and names, not `Expression` instances. Tags live as long as the plan does, so holding an `Expression` retains a whole expression subtree; render to a name or type at construction.
Structured reasons should also make [issue 144](https://github.com/apache/datafusion-comet/issues/144) (surface fallback reasons in the Spark UI) easier, since a UI wants to group and count reasons rather than print a bag of strings.
## Additional context
This does not need to be a single 205-call-site change. A staged migration keeps every intermediate state compiling and rendering identically:
1. Add the hierarchy including the `Other(String)` escape hatch, flip the tag to `Set[FallbackReason]`, and keep the existing `withFallbackReason(node, s: String)` signature as an overload that wraps in `Other`. Behaviour is unchanged at this point.
2. Convert call sites family by family (config-disabled, unsupported data type, unsupported expression/operator), tightening the corresponding tests from substring matches to typed matches as each family lands.
3. Once `Other` is rare, decide whether to remove the string overload or keep it for genuine one-offs.
The main thing to be careful about in step 1 is that rendering is preserved byte-for-byte, since both the golden extended-explain output and the substring assertions in `checkSparkAnswerAndFallbackReasons` depend on the current wording.
Contributor guide
Research direction
Start with withFallbackReason/withFallbackReasons and CometExplainInfo.FALLBACK_REASONS, then trace how ExtendedExplainInfo and the fallback log consume the rendered values. Review CometTestBase.checkSparkAnswerAndFallbackReasons and existing SupportLevel patterns before choosing the staged migration boundary. Done means structured reasons are stored and queryable while rendering remains byte-for-byte compatible with current output and tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala
- Domain
- backend
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100