andygrove opened a new pull request, #6379: URL: https://github.com/apache/datafusion-comet/pull/6379
## Which issue does this PR close? Closes #5203. Items 1 and 2 of the issue (`ReusedSubqueryExec` counted as an un-accelerated Spark operator, and `CometSubqueryBroadcastExec` counted as Spark) were fixed by #5206. This PR fixes the remaining item 3. ## Rationale for this change `ExtendedExplainInfo.generateTreeString` reaches every child through `CometExplainInfo.getActualPlan`, which unwraps a `ReusedExchangeExec` to the exchange it points at. A reused exchange subtree is therefore rendered and counted again in full at every reference, although it runs only once. That inflates the "Comet accelerated N out of M eligible operators" summary line, and the same counts feed `CometMetricsListener` -> `CometSource` (`operators.native`, `operators.spark`, `acceleration.ratio`). For example, TPC-DS q1 reported 48 of 48 operators. Its four-operator `date_dim` broadcast subtree is shown three times (once under the DPP `CometSubqueryBroadcast`, twice through exchange reuse), so only 40 operators actually run. ## What changes are included in this PR? - `CometCoverageStats` records the exchanges it has counted, compared by reference. Spark's `ReuseExchangeAndSubquery` points each `ReusedExchangeExec` at the same instance as the exchange it leaves in the plan, so the original and every reuse resolve to one entry. - `generateTreeString` counts an exchange subtree at whichever reference the traversal reaches first and renders the other references into throwaway stats. The reuse can come first: a node's subqueries are rendered before its children, and a subquery can hold the reuse of an exchange defined further down. The rendered tree is unchanged. - The "Understanding Comet Plans" guide described the double counting as a caveat. It now describes the new behavior. - Regenerated the TPC-DS plan stability goldens for Spark 3.4, 3.5, 4.0, 4.1 and 4.2 with `./dev/regenerate-golden-files.sh`. 142 of the 158 `extended.txt` files change, and in each one only the summary line changes. In 16 of them the transition count also drops, because the reused subtree contains a transition. In q58, for example, a scalar `Subquery` over a `CometColumnarToRow` sits inside a `CometBroadcastExchange` that is shown four times. The expression counts do not change. - One new golden, `approved-plans-v1_4-spark3_5/q33`. q33 renders the same tree on Spark 3.4 and 3.5, so it had no 3.5 copy. Now 3.4 counts 63 operators and 3.5 counts 55. Spark's own q33 plan has the same exchange reuse on both versions, and 55 matches it, so Comet's 3.4 plan seems to miss one reuse (8 operators) that the old double counting hid. ## How are these changes tested? - New test in `CometCoverageStatsSuite`: a `UnionExec` over a Comet shuffle exchange and a `ReusedExchangeExec` pointing at it, in both orders. It asserts that the exchange subtree is counted once and still rendered at both references. Without the fix it fails with `2 did not equal 1`. - `CometCoverageStatsSuite` and the two `CometSource` metrics tests in `CometPluginsSuite` pass on the default Spark 4.1 profile. The metrics tests only assert that counters increase, so they need no update. - The golden regeneration ran both plan stability suites on every profile (103 v1.4 and 32 v2.7 queries each), and all passed. A script over `git diff` confirmed that each changed golden differs by exactly one line, the summary line. No tree line changed, the expression counts are unchanged, and no count went up. - Before regenerating, I predicted the new summaries by simulating the fixed counting on the tree text, treating identical exchange subtrees as reuses. 95 of the 158 goldens match exactly. The rest fall between the prediction and the old count, because distinct exchanges that differ only in their predicates render identically. q2 is an example: its `d_year = 2001` and `d_year = 2002` `date_dim` broadcasts look the same but are not reused in Spark's own plan either. I checked q1 (40 of 40) and q2 (32 of 32) by hand. -- 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]
