docs: document ReusedExchange caveat and both CometSparkToColumnar names in operator-count exclusions - #5240
Merged
Conversation
…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.
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?
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.mdlisting the nodes excluded fromthe operator counts in the extended explain summary. Review raised two gaps in that
list.
What changes are included in this PR?
Both rendered names of
CometSparkToColumnarExecare listed.nodeNamereturnsCometSparkColumnarToColumnarwhenchild.supportsColumnaris true andCometSparkRowToColumnarotherwise, but the transition-nodes bullet only named thelatter. Both are already excluded today, because
generateTreeStringmatches on theScala type rather than the rendered name, so a reader who sees
CometSparkColumnarToColumnarin their own explain output could not tell from thelist 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.
ReusedExchangeis now covered, with its caveat. It is matched in the sameignore arm as
AdaptiveSparkPlanExec,InputAdapter,WholeStageCodegenExec,QueryStageExec, andAQEShuffleReadExec, but was not mentioned anywhere in thesection. It also does not behave like those wrappers:
getActualPlanunwraps it toits 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" --checkpasses. No new in-page anchor links were added: MyST slugifies theColumnar/Row Transitionsheading with the slash dropped, so a natural-looking#columnar-row-transitionslink would have been broken, and the two node names areexplained inline in the bullet instead.