andygrove opened a new pull request, #5236:
URL: https://github.com/apache/datafusion-comet/pull/5236
## Which issue does this PR close?
Closes #5230 (steps 1 and 2).
## Rationale for this change
Comet records fallback reasons in a `TreeNodeTag` side channel. Extended
explain output only walks plan nodes (`ExtendedExplainInfo.sortup` follows
`children`/`innerChildren`, never `expressions`), so an expression-level reason
is invisible unless something lifts it onto the enclosing operator.
That lifting was hand-written at roughly 200 call sites, and nothing forced
it. Forgetting the roll-up argument produced a plausible-looking generic
`"<operator> is not supported"` message instead of the real reason, which is
why the bug slipped through review twice before (#2323, #2716). The codebase
already contained the better design for the *other* tag:
`CometExecRule.rollUpInfoMessages` does the identical job once, centrally.
## What changes are included in this PR?
**Step 1 — strict mode.** `CometExecRule.reportUnexplainedFallback` replaces
the masking behaviour. When Comet declines an operator whose children are all
already native, and neither the operator nor any of its expressions carries a
reason, `spark.comet.explain.fallback.strict.enabled` (internal, default off)
throws instead of tagging the generic message. `CometTestBase` enables it, so
the whole test corpus now enforces it.
The check is deliberately scoped to operators a serde actually attempted. An
operator with no registered handler was never attempted, so demanding a
specific reason there would be wrong — it keeps the generic message. Same for
an operator whose children are not native: Comet had no opportunity, so there
is nothing to explain.
**Step 2 — central traversal.** `CometExecRule.rollUpFallbackReasons`
collects `FALLBACK_REASONS` from `op.expressions.flatMap(_.collect { ... })`
and tags them on the operator, at the single point where `convertToComet`
returns `None`. Two details from the issue:
- `hasFallbackReason` still reads **only** the node's own tag. It is a
planning control signal (`CometNativeScan`, `CometShuffleExchangeExec`,
`CometExecRule`), not explain output, and must not observe the traversal.
- The roll-up runs only for the operator that actually failed conversion.
That containment matters because `AttributeReference`s and DPP subquery
expressions are shared across operators, so an unscoped roll-up could surface
one expression's reason under several unrelated operators.
**API change that falls out of step 2.** With the central traversal in place
the roll-up parameters are dead, so they are removed rather than left as a trap:
- `withFallbackReason[T](node: T, info: String, exprs: T*)` →
`withFallbackReason[T](node: T, info: String)`
- `withFallbackReasons[T](node: T, info: Set[String], exprs: T*)` →
`withFallbackReasons[T](node: T, info: Set[String])`
- the pure-roll-up overload `withFallbackReason[T](node: T, exprs: T*)` is
deleted
- `QueryPlanSerde.optExprWithFallbackReason` is deleted (77 call sites; it
becomes the identity once the roll-up is gone)
The compiler now rejects any attempt to reintroduce a hand-rolled roll-up,
which also removes the type-safety hole in issue point 4 (the old signature
unified `T` to `TreeNode[_]`, so `withFallbackReason(op, op.condition,
op.child)` typechecked with an `Expression` and a `SparkPlan` in the same
varargs).
Also drops the `var allProjExprs` accumulator in `CometExpandExec` (issue
point 5), which existed only to have something to hand to the roll-up.
## How are these changes tested?
New tests in `CometExecRuleSuite`:
- an expression-level reason is rolled up onto the falling-back operator,
and the generic message does *not* also appear
- the strict config is off by default and on for Comet suites
- strict mode does not fire for operators Comet never attempted
Existing suites, all passing with strict mode on: `CometExecSuite`,
`CometExpressionSuite`, `CometAggregateSuite`, `CometJoinSuite`,
`CometWindowExecSuite`, `CometGenerateExecSuite`, `CometExecRuleSuite`,
`CometScanRuleSuite`, `CometSparkSessionExtensionsSuite`, `CometFuzzTestSuite`,
`CometFuzzAggregateSuite`, `CometCastSuite`, `CometArrayExpressionSuite`,
`CometStringExpressionSuite`, `CometShuffleSuite`, `CometNativeShuffleSuite`,
`CometShuffleFallbackStickinessSuite`, `CometDppFallbackRepro3949Suite`,
`CometCodegenSuite` — 1000 tests, 0 failures.
Compiles clean (main + test) on `spark-3.4`, `spark-3.5`, `spark-4.0` and
`spark-4.1`.
Not run locally: the TPC-DS plan stability suites need `SPARK_HOME`, which
isn't available in my environment. Worth watching in CI, since they assert on
the rendered `[COMET: ...]` segments — the per-node attribution of a rolled-up
reason can differ from the old snapshot-at-call-time behaviour.
Step 3 of #5230 (the `serializeExprs` combinator on `CometOperatorSerde`) is
not included; it is independent and easier to review separately.
--
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]
