docs: design for short-circuiting serde compatibility checks (#3990)

Author: andygrove

docs/superpowers/specs/2026-04-18-short-circuit-serde-checks-design.md

@@ -0,0 +1,114 @@
+# Short-circuit serde compatibility checks via per-handler attempt tag
+
+Tracking issue: [#3990](https://github.com/apache/datafusion-comet/issues/3990)
+
+## Goal
+
+Comet's plan rules run at both initial planning and on every AQE stage-prep
+pass. On each invocation the serde framework redoes the same operator
+compatibility checks. For operators that already fell back on a prior pass,
+`CometExecRule.isOperatorEnabled` re-invokes `handler.getSupportLevel(op)` —
+which for shuffle, aggregate, and window handlers also walks child
+expressions. This is wasted work and, per #3949, risks re-deriving against
+a reshaped subtree and flipping the answer.
+
+Eliminate the repeat cost for the operator path by recording, per operator
+node, which handler classes have already tried `getSupportLevel` and
+returned a non-matching support level. Subsequent passes short-circuit
+those handlers without re-running the check.
+
+## Non-goals
+
+- Expression-level short-circuit in `exprToProto` / `exprToProtoInternal.convert`.
+  The hot partitioning/aggregate expression walks called out in #3990 are
+  reached through operator handlers' `getSupportLevel`, so the operator-level
+  cache covers them indirectly. If profiling after this lands still shows
+  hot `exprToProto` walks, an expression-level cache is a follow-up.
+- Scan-level short-circuit in `CometScanRule`.
+- Positive caching of successful protobuf conversions. Successful conversions
+  replace the original operator with `CometNativeExec` after pass 1, so
+  pass 2 does not re-encounter them.
+- Standardizing every `withInfo` site on "tag only on total failure"
+  (direction B from #3990). That is a separate, larger audit.
+
+## Design
+
+### New tag
+
+Add a new `TreeNodeTag[Set[String]]` on `CometExplainInfo`:
+
+```scala
+val FAILED_HANDLERS = new TreeNodeTag[Set[String]]("CometFailedHandlers")
+```
+
+The set holds `handler.getClass.getName` for each handler that has already
+run `getSupportLevel` on this operator and returned `Unsupported` or
+`Incompatible` (without `allowIncompat`). The tag is orthogonal to the
+existing `EXTENSION_INFO` tag used by `withInfo` / explain output.
+
+### Change site 1 — `CometExecRule.convertToComet`
+
+At the top of `convertToComet(op, handler)`, before calling
+`isOperatorEnabled`, check whether `handler.getClass.getName` is already in
+`op`'s `FAILED_HANDLERS` set. If yes, return `None` immediately. No call to
+`isOperatorEnabled`, no call to `handler.getSupportLevel`, no expression
+walks inside `getSupportLevel`.
+
+### Change site 2 — `CometExecRule.isOperatorEnabled`
+
+On the two return-`false` paths inside the `getSupportLevel` match —
+`Unsupported` and `Incompatible` without `allowIncompat` — add
+`handler.getClass.getName` to `op`'s `FAILED_HANDLERS` tag.
+
+Do **not** record on the `enabledConfig` short-circuit path. That check
+is a fast config lookup and Spark config can toggle between passes; caching
+it yields no speedup and risks stale decisions.
+
+### Why this preserves the `get_struct_field` / multi-handler case
+
+The cache is keyed per handler class. A scan tagged by `CometScanRule`'s
+scan handler only records *that* handler class in `FAILED_HANDLERS`. When
+`CometExecRule` later tries `CometSparkToColumnarExec` on the same node,
+its handler class is not in the set, so it proceeds normally. This is the
+case that #3990 explicitly warned would break under a coarse
+`hasExplainInfo` short-circuit, and it is preserved here.
+
+### Interaction with existing `withInfo` / `EXTENSION_INFO`
+
+Unchanged. `withInfo` continues to record fallback reasons for extended
+explain output. The new `FAILED_HANDLERS` tag is internal, written and
+read only by `convertToComet` / `isOperatorEnabled`, and does not affect
+explain output or `hasExplainInfo`.
+
+## Testing
+
+- **Unit test** (new, in `CometExecRuleSuite` or adjacent): construct an
+  operator and a counting stub handler that returns `Unsupported`. Call
+  `convertToComet` twice; assert `getSupportLevel` is invoked exactly once
+  and the second call returns `None` directly.
+- **Regression**: `CometExpressionSuite "get_struct_field"` must still pass.
+  This is the test #3990 explicitly flagged as breaking under a coarse
+  short-circuit. It validates that the per-handler keying preserves the
+  scan-tagged-node + `CometSparkToColumnarExec` case.
+- **Behavioral**: a query that goes through two rule passes (initial plan +
+  one AQE stage-prep pass) on an operator that falls back. Assert final
+  plan is identical and `withInfo` reasons don't double-accumulate.
+  `withInfos` already dedups via `Set`, so this is a guard against
+  regression rather than new behavior.
+- **CI**: full JVM suite runs in CI. Local verification uses a targeted
+  subset (primarily `CometExpressionSuite` plus any suite exercising AQE
+  stage-prep re-runs).
+
+## Risks
+
+- **AQE re-planning and node identity.** If AQE produces a fresh operator
+  instance rather than re-running rules on the same instance, the tag does
+  not carry and the work is redone. That matches current behavior — no
+  regression, just less speedup than if identity were preserved. Worth
+  noting; not blocking.
+- **Config changes between passes.** Handled explicitly by skipping the
+  `enabledConfig` path.
+- **Partial-path tagging elsewhere.** #3990 warns that some serde paths
+  record fallback reasons even when another path could still succeed.
+  This design sidesteps that risk by being per-handler, not relying on
+  `hasExplainInfo`. The fix for partial-path tagging is out of scope.