andygrove opened a new issue, #5203:
URL: https://github.com/apache/datafusion-comet/issues/5203
## 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
```scala
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.
--
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]
