Skip to content

docs: document ReusedExchange caveat and both CometSparkToColumnar names in operator-count exclusions - #5240

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:docs-explain-count-exclusions
Aug 3, 2026
Merged

docs: document ReusedExchange caveat and both CometSparkToColumnar names in operator-count exclusions#5240
andygrove merged 1 commit into
apache:mainfrom
andygrove:docs-explain-count-exclusions

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

Docs follow-up to review feedback on #5206, which was deferred to keep that PR's CI
scope small.

Rationale for this change

#5206 added a section to understanding-comet-plans.md listing the nodes excluded from
the operator counts in the extended explain summary. Review raised two gaps in that
list.

What changes are included in this PR?

  1. Both rendered names of CometSparkToColumnarExec are listed. nodeName returns
    CometSparkColumnarToColumnar when child.supportsColumnar is true and
    CometSparkRowToColumnar otherwise, but the transition-nodes bullet only named the
    latter. Both are already excluded today, because generateTreeString matches on the
    Scala type rather than the rendered name, so a reader who sees
    CometSparkColumnarToColumnar in their own explain output could not tell from the
    list that it was covered. The bullet now names both and says they are one operator
    under two names. This matches the Columnar/Row Transitions table further down, which
    already documents both names.

  2. ReusedExchange is now covered, with its caveat. It is matched in the same
    ignore arm as AdaptiveSparkPlanExec, InputAdapter, WholeStageCodegenExec,
    QueryStageExec, and AQEShuffleReadExec, but was not mentioned anywhere in the
    section. It also does not behave like those wrappers: getActualPlan unwraps it to
    its child, so the reused subtree is counted once per reference rather than once for
    the whole plan. Since the section exists to explain what is excluded and why, the new
    bullet states that the node itself is skipped, spells out the per-reference
    double-counting with a concrete example, and points at item 3 of Extended explain operator stats miscount reuse wrappers and CometSubqueryBroadcast #5203 as the
    tracking issue.

No prose was removed, and no non-docs files are touched.

How are these changes tested?

Docs-only. npx prettier "docs/source/user-guide/latest/understanding-comet-plans.md" --check passes. No new in-page anchor links were added: MyST slugifies the
Columnar/Row Transitions heading with the slash dropped, so a natural-looking
#columnar-row-transitions link would have been broken, and the two node names are
explained inline in the bullet instead.

…mes in operator-count exclusions

Follow-up to review feedback on apache#5206.

- The transition-nodes bullet only listed `CometSparkRowToColumnar`, but
  `CometSparkToColumnarExec` also renders as `CometSparkColumnarToColumnar`
  when its child supports columnar. Both names are excluded from the operator
  counts today (the match is on the Scala type, not the rendered name), so list
  both.

- `ReusedExchangeExec` is matched in the same ignore arm as the other wrapper
  nodes but was not mentioned. It also behaves differently from them:
  `getActualPlan` unwraps it to its child, so a reused subtree is counted once
  per reference rather than once per plan. Document it along with that caveat
  and the link to item 3 of apache#5203.
@andygrove
andygrove requested a review from mbutrovich August 3, 2026 22:11
@andygrove andygrove added this to the 1.0.0 milestone Aug 3, 2026

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @andygrove!

@andygrove
andygrove merged commit af534e0 into apache:main Aug 3, 2026
17 checks passed
@andygrove
andygrove deleted the docs-explain-count-exclusions branch August 3, 2026 22:19
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.

2 participants