Skip to content

refactor: replace hand-coded rollup of expression fallback reasons onto operators - #5236

Open
andygrove wants to merge 4 commits into
apache:mainfrom
andygrove:central-fallback-reason-rollup
Open

refactor: replace hand-coded rollup of expression fallback reasons onto operators#5236
andygrove wants to merge 4 commits into
apache:mainfrom
andygrove:central-fallback-reason-rollup

Conversation

@andygrove

Copy link
Copy Markdown
Member

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 AttributeReferences 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.

…raversal

Comet records fallback reasons in a `TreeNodeTag` side channel. Extended
explain output only walks plan nodes, so an expression-level reason is
invisible unless something lifts it onto the enclosing operator. That
lifting was hand-written at ~200 call sites, and nothing forced it, so
forgetting the roll-up argument silently produced a plausible-looking
generic message instead of the real reason (fixed twice before, in apache#2323
and apache#2716).

Steps 1 and 2 of apache#5230:

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 `"<operator> is not supported"`.
`CometTestBase` enables it, so the whole test corpus now enforces it. The
check is scoped to operators a serde actually attempted; an operator with
no registered handler was never attempted and still gets the generic
message.

Central traversal. `CometExecRule.rollUpFallbackReasons` collects
`FALLBACK_REASONS` from `op.expressions` and tags them on the operator at
the single point where Comet decides to keep the Spark operator, mirroring
the existing `rollUpInfoMessages`. It only runs on the operator that
failed conversion, which contains the shared-instance problem for
`AttributeReference`s and DPP subquery expressions. `hasFallbackReason`
still reads only the node's own tag: it is a planning control signal and
must not observe the traversal.

With that in place the roll-up parameters are dead, so they are gone from
the API: `withFallbackReason(node, info)` and
`withFallbackReasons(node, info)` no longer take varargs, and the
pure-roll-up overload and `optExprWithFallbackReason` are deleted. The
compiler now rejects any attempt to reintroduce a hand-rolled roll-up, and
the old signature's lack of type safety (issue point 4) goes with it. Also
drops the `var allProjExprs` accumulator in `CometExpandExec`, which
existed only to feed the roll-up.

Tested: CometExecSuite, CometExpressionSuite, CometAggregateSuite,
CometJoinSuite, CometWindowExecSuite, CometGenerateExecSuite,
CometExecRuleSuite, CometScanRuleSuite, CometSparkSessionExtensionsSuite,
CometFuzzTestSuite, CometFuzzAggregateSuite, CometCastSuite,
CometArrayExpressionSuite, CometStringExpressionSuite, CometShuffleSuite,
CometNativeShuffleSuite, CometShuffleFallbackStickinessSuite,
CometDppFallbackRepro3949Suite, CometCodegenSuite - all pass with strict
mode on. Compiles clean on spark-3.4, 3.5, 4.0 and 4.1.
scalafix RemoveUnused flagged `val allExprs = list ++ Seq(value)`, which
only existed to feed the deleted roll-up call.
Strict mode caught a real pre-existing hole. `exprToProto` runs
`DecimalPrecision.promote`, and `transformUp` rebuilds every node on the
path to a rewritten one, so for decimal arithmetic the nodes that serde
actually converts are copies rather than the nodes in the plan. Any reason
recorded during conversion landed on a copy, where neither extended
explain nor the operator roll-up could ever see it.

The old hand-written roll-up did not find these either - it read the
original `projectList`, so the reason was equally lost - which is why this
only surfaced now: previously the empty tag still rendered as a bare
`[COMET: ]` and nobody noticed. TPC-DS q9's approved plan records exactly
that, and is updated here to carry the real reason instead.

Copy the reasons from the rewritten tree onto the original node, the same
copy-back the `Invoke` / `StaticInvoke` rewrites in `Spark4xCometExprShim`
already do.

Repro (all Spark versions, [exec] and [expressions] CI shards):
  INSERT INTO t SELECT CAST(id AS decimal(18,4)) + 0.0001 FROM range(20000)
threw "Comet did not convert Project but recorded no fallback reason".

Also drops an empty `if (r.isEmpty) {}` block left in CometAlias by the
roll-up removal.

Verified: CometSqlFileTestSuite, the full [expressions] shard (1127 tests)
and [exec] shard (508 tests) on Spark 4.0, and both TPC-DS plan stability
suites (129 tests) on Spark 3.4, 3.5, 4.0 and 4.1 - all pass. Plan
stability needs spark.test.home pointed at the Comet repo root to run.
Resolves a conflict in `exprToProto`. Main's apache#5201 independently hit the
same problem this branch fixes: `DecimalPrecision.promote` rebuilds the
tree, so tags recorded during conversion land on copies the operator does
not hold. Main added `liftCoverageTags` for the coverage tags; this branch
added the same copy-back for fallback reasons. Keep both, as
`liftCoverageTags` plus a new `liftFallbackReasons`.

The two differ in when they run: coverage tags lift unconditionally, while
fallback reasons lift only when conversion failed, because a reason states
why an expression could not be converted and lifting one off a tree that
converted fine would attribute a stale reason to a healthy operator.
@andygrove
andygrove marked this pull request as ready for review August 4, 2026 00:15
@andygrove
andygrove requested a review from parthchandra August 4, 2026 14:30
.flatten
.toSet
if (reasons.nonEmpty) {
withFallbackReasons(op, reasons)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We roll up the expression fallback reasons to the operator once in the framework instead of hand-coding it for every single operator

@andygrove

Copy link
Copy Markdown
Member Author

@parthchandra could you review this one, since you are familiar with this code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace hand-coded rollup of expression fallback reasons onto operators

1 participant