refactor: replace hand-coded rollup of expression fallback reasons onto operators - #5236
Open
andygrove wants to merge 4 commits into
Open
refactor: replace hand-coded rollup of expression fallback reasons onto operators#5236andygrove wants to merge 4 commits into
andygrove wants to merge 4 commits into
Conversation
…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
marked this pull request as ready for review
August 4, 2026 00:15
andygrove
commented
Aug 4, 2026
| .flatten | ||
| .toSet | ||
| if (reasons.nonEmpty) { | ||
| withFallbackReasons(op, reasons) |
Member
Author
There was a problem hiding this comment.
We roll up the expression fallback reasons to the operator once in the framework instead of hand-coding it for every single operator
Member
Author
|
@parthchandra could you review this one, since you are familiar with this code? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #5230 (steps 1 and 2).
Rationale for this change
Comet records fallback reasons in a
TreeNodeTagside channel. Extended explain output only walks plan nodes (ExtendedExplainInfo.sortupfollowschildren/innerChildren, neverexpressions), 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.rollUpInfoMessagesdoes the identical job once, centrally.What changes are included in this PR?
Step 1 — strict mode.
CometExecRule.reportUnexplainedFallbackreplaces 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.CometTestBaseenables 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.rollUpFallbackReasonscollectsFALLBACK_REASONSfromop.expressions.flatMap(_.collect { ... })and tags them on the operator, at the single point whereconvertToCometreturnsNone. Two details from the issue:hasFallbackReasonstill 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.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])withFallbackReason[T](node: T, exprs: T*)is deletedQueryPlanSerde.optExprWithFallbackReasonis 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
TtoTreeNode[_], sowithFallbackReason(op, op.condition, op.child)typechecked with anExpressionand aSparkPlanin the same varargs).Also drops the
var allProjExprsaccumulator inCometExpandExec(issue point 5), which existed only to have something to hand to the roll-up.How are these changes tested?
New tests in
CometExecRuleSuite: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.0andspark-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
serializeExprscombinator onCometOperatorSerde) is not included; it is independent and easier to review separately.