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