andygrove opened a new issue, #5230:
URL: https://github.com/apache/datafusion-comet/issues/5230
## What is the problem the feature request solves?
Comet's serde code records fallback reasons in a `TreeNodeTag` side channel
(`CometExplainInfo.FALLBACK_REASONS`). `withFallbackReasons` in
`CometSparkSessionExtensions.scala` does two jobs at once: it records the
node's own reasons, and it *eagerly copies* the tag values off the child nodes
passed in `exprs`.
That copy is necessary because extended explain output only walks plan nodes
— `ExtendedExplainInfo.sortup` follows `children`/`innerChildren`, never
`expressions`. An expression-level reason is therefore invisible unless
something lifts it onto the enclosing operator. Today that lifting is
hand-written at roughly 200 call sites, and about 118 of them pass no message
at all and exist purely to roll up:
```scala
withFallbackReason(op, op.projectList: _*) // CometProjectExec
withFallbackReason(op, op.condition, op.child) // CometFilterExec
withFallbackReason(expr, left, right) //
QueryPlanSerde.createBinaryExpr
```
This is repetitive and easy to get wrong. Concrete problems:
1. **Nothing forces it.** `convert` returns `Option`, so returning `None`
without tagging a reason compiles fine. This has shipped as a bug at least
twice, both fixed by adding the forgotten roll-up argument:
- PR #2323 — "TakeOrderedAndProjectExec is not reporting all fallback
reasons"
- PR #2716 — "missing SortOrder fallback reason in range partitioning"
2. **The catch-all masks it.** `CometExecRule` tags a generic
`"${op.nodeName} is not supported"` when an unconverted operator has no reason.
A missing roll-up therefore produces a plausible-looking but useless message
rather than a visible hole, which is why these slip through review.
3. **Snapshot, not reference.** Reasons are copied at call time, so anything
tagged on a child *after* the parent rolled up is silently dropped. The
conventional shape (convert children, then roll up in the `else` branch) is
safe, but nothing marks that ordering as load-bearing.
4. **The signature gives no type safety.** `withFallbackReason[T <:
TreeNode[_]](node: T, exprs: T*)` unifies `T` to `TreeNode[_]` as soon as kinds
are mixed, so `withFallbackReason(op, op.condition, op.child)` typechecks with
an `Expression` and a `SparkPlan` in the same varargs. There is no check that
the passed nodes are related to `node` at all.
5. **It distorts serde code.** `CometExpandExec` carries a `var
allProjExprs`, accumulated inside a `flatMap`, purely so it has something to
hand to the roll-up.
6. **The tag is dual-purpose.** `hasFallbackReason` is a control signal, not
only explain output (`CometNativeScan`, `CometShuffleExchangeExec`,
`CometExecRule`). So this cannot be fixed by simply rolling up more
aggressively — over-rolling changes planning decisions.
7. **The codebase already contains the better design, for the other tag.**
`CometExecRule.rollUpInfoMessages` handles `EXTENSION_INFO` and
`CODEGEN_DISPATCH_EXPRS` in one central place by walking
`op.expressions.flatMap(_.collect { case e: Expression => e })`. Its doc
comment even explains why ("explain only traverses plan nodes, not
expressions"). The fallback path does the identical job 200 times by hand.
## Describe the potential solution
Proposed in order of increasing cost. The first three are complementary; the
last two are alternatives to consider.
### Step 1 — Strict mode (cheap, independent, do first)
Replace the masking behaviour in `CometExecRule` with a test-only strictness
config: when an operator is left unconverted and neither it nor any of its
expressions carries a fallback reason, throw. Enable it in `CometTestBase` for
all suites.
This turns the whole bug class into test failures and is a prerequisite for
trusting the refactors below.
### Step 2 — Central traversal instead of per-site roll-up (the structural
fix)
Mirror `rollUpInfoMessages`: add `rollUpFallbackReasons(op)` that collects
`FALLBACK_REASONS` from `op.expressions.flatMap(_.collect { ... })`, and call
it at the single point where `CometExecRule` decides to keep the Spark
operator. Every pure roll-up call site then deletes, and serdes only write real
reasons.
Two things to get right:
- Keep `hasFallbackReason` reading only the node's own tag. It is the
control signal and must not observe the traversal.
- Verify the shared-instance question: `AttributeReference`s and DPP
subquery expressions are shared across operators, so a reason tagged on one
could surface under several. Scoping the roll-up to the operator that actually
failed conversion — which is what the current code effectively does — contains
this.
### Step 3 — A combinator, so the roll-up stops being a separate step
Most operator serdes have the same shape. Add a helper to
`CometOperatorSerde`:
```scala
protected def serializeExprs[E <: Expression](
op: SparkPlan, exprs: Seq[E], inputs: Seq[Attribute])(
f: Seq[Expr] => Option[Operator]): Option[Operator]
```
It converts the expressions and, on any failure, rolls up exactly the exprs
it was handed and returns `None`. `CometProjectExec`, `CometFilterExec`,
`CometExpandExec` and the join serdes lose their `else` branches entirely, and
the `var allProjExprs` goes away. `QueryPlanSerde.optExprWithFallbackReason` is
already a partial version of this for expressions; it is just used
inconsistently.
After steps 2 and 3, the only thing a serde author has to remember is to
state a real reason — and step 1 enforces that.
## Additional context
Two further options, recorded for completeness:
**Return the reason instead of tagging it** — change serde conversion to
`Either[Seq[FallbackReason], Expr]`. Failure then carries its reason, the
framework composes child reasons into the parent's, and the compiler makes
silent failure impossible; tags become a write-only rendering detail at the
explain boundary. This is the right end state, but it touches both serde traits
and roughly 35 files, so it is better bundled with a serde API break than done
on its own.
**Ambient collector scope** — have `exprToProtoInternal` append failures to
a conversion-scoped collector opened by `convertToComet`, attaching everything
collected in scope to the operator on failure. Same outcome as step 2 without
touching serde signatures, but it introduces hidden mutable state and needs
care around reentrancy and nested AQE plans. Step 2 is preferable because it
derives the same result from data already present in the tree.
--
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]
