apache / apache/datafusion-comet
Codegen dispatcher is unreachable from `convert`, so serdes that decline there never get a dispatch attempt
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
Part of #5572.
The dispatcher is only reachable from `getSupportLevel`. `dispatchIfFallback` is called from exactly two places, the `Unsupported` arm (`spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:941`) and the non-opt-in `Incompatible` arm (`:968`). The `Compatible` arm just calls `handler.convert(...)` and propagates whatever it returns.
So a serde that reports `Compatible` and *then* declines inside `convert` gets no dispatch attempt, even when it mixes in `CodegenDispatchFallback`. Around 15 serdes decline that way via `withFallbackReason`, including two that already carry the mixin and therefore silently lose it on that path:
- `CometJsonToStructs` (`serde/structs.scala:195`)
- `CometArrayReverse` (`serde/arrays.scala:598`)
and, without the mixin, `CometCreateArray`, `CometGetArrayItem`, `CometGetArrayStructFields`, `CometArrayInsert`, `CometStaticInvoke`, `CometUuid`, `CometShuffle`, `CometAttributeReference`, `CometScalarFunction`.
`CometUnixTimestamp` is the clearest illustration: it makes the identical input-type check twice, once in `getSupportLevel` (`serde/datetime.scala:319`) and once in `convert` (`:329`). Only the first copy would benefit from adding the mixin.
Two ways to close this:
1. Move the checks into `getSupportLevel` serde by serde. Precise, no behavior change anywhere else, but it is a lot of small edits and nothing stops the pattern coming back.
2. Add `.orElse(dispatchIfFallback(handler, expr, inputs, binding).map(_._2))` after `handler.convert` in the `Compatible` arm. One line, and it fixes the pattern permanently.
Option 2 needs care. A `Compatible` serde's `convert` can legitimately return `None` because a *child* failed to serialize, not because of anything wrong with this node, and in that case retrying the whole subtree through the dispatcher is a real behavior change rather than a bug fix — though arguably a desirable one, since the dispatcher can evaluate a subtree containing an expression that has no native serde at all. It also needs the guard in the prerequisite issue first. Worth prototyping option 2 and measuring how often it changes a plan across the TPC-DS and Spark SQL suites before committing to it.
Related: a warning when `getSupportLevel` returns `Compatible` and `convert` then returns `None` would make the size of this surface visible, since that combination is a serde invariant violation whenever the cause is the node itself rather than a child.
Contributor guide
Research direction
Start in spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala at the Unsupported, Incompatible, and Compatible arms around lines 941 and 968. Inspect the listed serdes, especially CometUnixTimestamp, CometJsonToStructs, and CometArrayReverse, then prototype the alternatives after the prerequisite issue is addressed. Run the TPC-DS and Spark SQL suites to measure plan changes before deciding what done means.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala, spark
- Domain
- backend, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100