mbutrovich commented on code in PR #5206: URL: https://github.com/apache/datafusion-comet/pull/5206#discussion_r3707877876
########## docs/source/user-guide/latest/understanding-comet-plans.md: ########## @@ -178,6 +178,17 @@ inline in the UI. Earlier Spark versions do not have the `extendedExplainProviders` extension point, so this provider is not used and the config has no effect there. +Not every node in the plan is an eligible operator. The following are excluded +from both operator counts: + +- Transition nodes (`CometColumnarToRow`, `CometNativeColumnarToRow`, + `CometSparkRowToColumnar`, `ColumnarToRow`, `RowToColumnar`), which are + reported separately as the transition count. +- Wrappers that do no work of their own: `AdaptiveSparkPlan`, `InputAdapter`, Review Comment: `CometSparkToColumnarExec.nodeName` renders as either `CometSparkColumnarToColumnar` or `CometSparkRowToColumnar`, depending on whether `child.supportsColumnar` is true. The transition-nodes bullet only lists `CometSparkRowToColumnar`. Since `generateTreeString` matches on the Scala type rather than the rendered name, both variants are already excluded from the operator counts today. Could the bullet list both names, so a reader who sees `CometSparkColumnarToColumnar` in their own explain output can tell it's covered by this list too? ########## docs/source/user-guide/latest/understanding-comet-plans.md: ########## @@ -178,6 +178,17 @@ inline in the UI. Earlier Spark versions do not have the `extendedExplainProviders` extension point, so this provider is not used and the config has no effect there. +Not every node in the plan is an eligible operator. The following are excluded +from both operator counts: + +- Transition nodes (`CometColumnarToRow`, `CometNativeColumnarToRow`, + `CometSparkRowToColumnar`, `ColumnarToRow`, `RowToColumnar`), which are + reported separately as the transition count. +- Wrappers that do no work of their own: `AdaptiveSparkPlan`, `InputAdapter`, + `WholeStageCodegen`, query stages, and `AQEShuffleRead`. +- The reuse marker `ReusedSubquery`. The subquery it points at is counted where + that subquery is shown, so the marker itself does not add to the totals. Review Comment: `ReusedExchangeExec` is matched in the same ignore arm as `AdaptiveSparkPlanExec`, `InputAdapter`, `WholeStageCodegenExec`, `QueryStageExec`, and `AQEShuffleReadExec`, but it isn't mentioned anywhere in this new section. It also behaves differently from the wrappers that are listed: `getActualPlan` unwraps it to its child, so a reused exchange subtree is counted once per reference rather than once total (the item 3 double-counting left open in #5203). Since this section's purpose is explaining what's excluded from the counts and why, could it also cover `ReusedExchangeExec`, including the caveat that it isn't cleanly excluded the way the others are? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
