andygrove opened a new issue, #5346:
URL: https://github.com/apache/datafusion-comet/issues/5346
## 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.
--
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]
