andygrove opened a new pull request, #5206:
URL: https://github.com/apache/datafusion-comet/pull/5206
## Which issue does this PR close?
Closes #5203.
Stacked on #5201 (`explain-expression-coverage`) because that PR also
regenerates every
TPC-DS `extended.txt` golden. Review that one first; the base will be
retargeted to `main`
once it merges.
## Rationale for this change
The operator coverage summary produced by `ExtendedExplainInfo` counted
three node kinds
incorrectly, so it understated acceleration on most TPC-DS plans and
massively overcounted
operators wherever an exchange was reused. The same counts feed the `comet`
metrics source
via `CometMetricsListener` -> `CometSource`, so the skew also biased the
`acceleration.ratio` gauge.
## What changes are included in this PR?
All three items from the issue:
1. **`ReusedSubqueryExec` is no longer counted as an un-accelerated Spark
operator.** It is
pure reuse bookkeeping and a `LeafExecNode`, so it now joins the ignore
list.
2. **`CometSubqueryBroadcastExec` now mixes in `CometPlan`** and counts as
accelerated. The
other `CometPlan` match sites (`CometExecRule`,
`EliminateRedundantTransitions`,
`foreachUntilCometInput`, `CometShuffleExchangeExec.isCometPlan`) all
reach nodes through
`children`, and a `BaseSubqueryExec` hangs off
`innerChildren`/expressions, so none of
them change behaviour. The one that would is `CometExecRule`'s "never
replace" guard,
which is the correct classification anyway.
3. **Reused exchanges are counted once.** `getActualPlan` unwrapped
`ReusedExchangeExec` to
its child, so the reused subtree was traversed and counted in full at
every reference. The
coverage traversal now uses a new
`CometExplainInfo.actualPlanForCoverage`, which is
`getActualPlan` minus that unwrapping. Because `ReusedExchangeExec` is a
`LeafExecNode`,
keeping the wrapper renders a `ReusedExchange` leaf and stops the walk,
matching how
`ReusedSubqueryExec` is handled and keeping the rendered tree consistent
with the counts
below it. `getActualPlan` itself is unchanged, so fallback-reason
collection still
descends into reused subtrees.
Effect on the goldens: 143 `extended.txt` files change, and 107 of them now
report 100%
acceleration where only 6 did before. q1 is the example from the issue:
```diff
- : : : +- CometBroadcastExchange
- : : : +- CometProject
- : : : +- CometFilter
- : : : +- CometNativeScan parquet
spark_catalog.default.date_dim
+ : : : +- ReusedExchange
...
-Comet accelerated 47 out of 49 eligible operators (95%). ...
+Comet accelerated 40 out of 40 eligible operators (100%). ...
```
Eight plans report a *lower* percentage than before, which is the double
counting being
removed: the reused subtrees were fully accelerated, so counting them once
per reference
inflated the ratio. The clearest case is `approved-plans-v2_7/q14a`, which
went from
`2167 out of 2302` to `127 out of 128` — that plan does not contain 2302
operators.
Also documents in `understanding-comet-plans.md` which nodes are excluded
from the counts.
## How are these changes tested?
New `CometCoverageStatsSuite` covers each of the three cases directly: a
`ReusedSubquery`
contributes nothing, a `ReusedExchange` contributes nothing at the reference
site while the
exchange it points at is still counted where it is defined, and wrapping a
plan in
`CometSubqueryBroadcastExec` adds exactly one accelerated operator.
TPC-DS plan stability goldens were regenerated for Spark 3.4, 3.5, 4.0, 4.1
and 4.2, and both
suites were then re-run in verification mode against the regenerated goldens
on all five
versions (129 tests each, all passing). `CometExecSuite`,
`CometCodegenSuite` and the new
suite also pass on the default profile (224 tests).
--
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]
