refactor: replace hand-coded rollup of expression fallback reasons onto operators - #5236
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.
| .flatten | ||
| .toSet | ||
| if (reasons.nonEmpty) { | ||
| withFallbackReasons(op, reasons) |
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
|
@parthchandra could you review this one, since you are familiar with this code? |
parthchandra
left a comment
There was a problem hiding this comment.
lgtm with some minor comments.
| }.sum | ||
| } | ||
|
|
||
| test("expression-level fallback reasons are rolled up onto the operator that falls back") { |
There was a problem hiding this comment.
Should we add a unit test that forces a handled operator to return None with no reason and asserts that an exception is thrown in strict mode?
| // reasons recorded on its expressions onto the operator itself - see | ||
| // `rollUpFallbackReasons` for why this is needed - and make sure something was recorded. | ||
| rollUpFallbackReasons(op) | ||
| reportUnexplainedFallback(op) |
There was a problem hiding this comment.
Is it correct to say that reportUnexplainedFallback relies on rollUpFallbackReasons to have rolled up the expression tags first? Should we add a comment to make sure a future change from separating them?
… dependency Addresses review feedback on apache#5236: - add a test that drives `reportUnexplainedFallback` with the exact shape a serde produces when it returns None without recording a reason (a handled operator over native children, no tag anywhere), asserting the strict-mode throw and the generic message when strict mode is off. No serde in the tree reaches that state - which is what the check enforces - so the operator is constructed by hand and the method is now package-visible. - document that `reportUnexplainedFallback` must run after `rollUpFallbackReasons`, since it reads only the operator's own tag.
|
Thanks @parthchandra. Good feedback. I addressed it. |
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.