Skip to content

Extended explain operator stats miscount reuse wrappers and CometSubqueryBroadcast #5203

Description

@andygrove

Describe the bug

The operator coverage summary produced by ExtendedExplainInfo (Comet accelerated N out of M eligible operators (X%)) counts three node kinds incorrectly, so it understates acceleration on most TPC-DS plans and double-counts reused exchanges.

All of the following are in ExtendedExplainInfo.generateTreeString, spark/src/main/scala/org/apache/comet/ExtendedExplainInfo.scala.

1. ReusedSubqueryExec is counted as an un-accelerated Spark operator

The ignore list at line 107 covers AdaptiveSparkPlanExec | InputAdapter | QueryStageExec | WholeStageCodegenExec | ReusedExchangeExec | AQEShuffleReadExec. ReusedSubqueryExec is not in it, is not a transition type, and is not a CometPlan, so it falls through to case _ => planStats.sparkOperators += 1 (line 116).

getActualPlan (line 234) also does not unwrap it, and ReusedSubqueryExec is a LeafExecNode (children is Nil), so the subquery plan it points at is never traversed. Net effect per reuse site: one phantom un-accelerated operator, and the native plan behind it contributes nothing.

2. CometSubqueryBroadcastExec is counted as a Spark operator

CometSubqueryBroadcastExec is declared

case class CometSubqueryBroadcastExec(...)
    extends BaseSubqueryExec
    with UnaryExecNode {

(spark/src/main/scala/org/apache/spark/sql/comet/CometSubqueryBroadcastExec.scala) and never mixes in CometPlan, so despite being Comet's own DPP operator it lands in the Spark bucket at line 116.

3. Reused exchanges are counted twice (inconsistent with the above)

ReusedExchangeExec is both ignored by the match and unwrapped by getActualPlan to its child (line 240), so the reused subtree is traversed and counted again in full at every reference.

The two reuse wrappers therefore skew in opposite directions: exchange reuse inflates the operator counts, subquery reuse deflates the acceleration percentage.

Steps to reproduce

Any plan with DPP or a reused scalar subquery. The checked-in goldens show it directly — spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q1/extended.txt:

Comet accelerated 47 out of 49 eligible operators (95%). Final plan contains 1 transitions between Spark and Comet.

That tree has 50 nodes. One is a transition (CometColumnarToRow), leaving 49 eligible, reported as 47 Comet plus 2 Spark. The two "Spark" operators are exactly ReusedSubquery and CometSubqueryBroadcast; every other node in the plan is a Comet* operator mixing in CometPlan. The plan is fully accelerated but reports 95%.

Prevalence across the 158 checked-in extended.txt goldens:

  • ReusedSubquery appears in 78
  • CometSubqueryBroadcast appears in 130
  • ReusedExchange appears in 0, because getActualPlan always unwraps it — which is what makes the double counting invisible in the goldens

Expected behavior

ReusedSubqueryExec is pure bookkeeping, not an operator that does work, so it should be ignored like ReusedExchangeExec rather than counted as un-accelerated. CometSubqueryBroadcastExec should count as a Comet operator. Reused exchanges should be counted once, not once per reference.

Additional context

The same counts feed the comet metrics source via CometMetricsListener -> CometSource (operators.native, operators.spark, acceleration.ratio), so the skew is not confined to EXPLAIN output — it also biases the acceleration ratio gauge.

Two open design questions for whoever picks this up:

  1. For CometSubqueryBroadcastExec, mixing in CometPlan is the tidier fix but has a wider blast radius, since other rules pattern-match on CometPlan. Special-casing it in the explain match is contained but leaves the type lying about itself. Worth checking the other CometPlan match sites before choosing.
  2. Fixing item 3 means tracking already-visited exchange subtrees during the traversal, which is more involved than items 1 and 2 and could reasonably be split out.

Any of these changes the operator counts, so the TPC-DS plan stability goldens will need regenerating for all supported Spark versions (./dev/regenerate-golden-files.sh).

Found while working on #5201, which adds expression-level coverage to the same summary line but does not touch the operator categorization.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingpriority:lowMinor issues, test failures, tooling, cosmeticrequires-triage

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions