andygrove opened a new pull request, #5514: URL: https://github.com/apache/datafusion-comet/pull/5514
## Which issue does this PR close? Closes #5335. Alternate approach to #5394 (which was itself an alternate to #5345). Heads up: I used an LLM to help draft this. The design is mine, but the code and prose have been shaped with LLM assistance, so review with that in mind. ## Rationale for this change Users evaluating Comet on a workload need a way to estimate how much of it Comet would accelerate without actually changing execution. Turning Comet on and comparing runs carries real risk. #5394 does this by reporting from inside `CometExecRule`. That works, but Spark applies a planner rule many times for one query — once per query stage and once per adaptive re-optimization under AQE, plus once for every subquery it prepares separately — so most of that PR is machinery deciding which application owns the report: a bounded LRU of execution-id/plan-hash keys, a tag copied along Catalyst rewrites, a query-stage check, an empty-re-plan check, and a `queryStagePrep` flag threaded in from the extension. Five review findings on that PR are variations of "the wrong application claimed the report", and each needed another guard. This PR reports from a `QueryExecutionListener` instead. One callback per action, holding the finished plan, so there is nothing to tell apart and none of that machinery exists. ## What changes are included in this PR? - New config `spark.comet.explain.planOnly.enabled`, default off. - `CometScanRule` and `CometExecRule` return the plan untouched at the top of `apply` while the mode is on, so Spark plans and executes the query exactly as it would with Comet off. `CometExecRule` also registers the session's listener, so a session carries one only if the mode is used, and the config stays togglable mid-session. - `CometPlanOnly` builds the report from `qe.executedPlan`: it undoes the preparation that follows the conversion rules (adaptive wrappers, query stages, `AQEShuffleReadExec`, codegen wrappers, columnar transitions, exchange reuse), previews the plans behind subquery expressions, converts, then replays transition insertion and Comet's post-columnar rules — so a stage Comet would have handed back to Spark is reported as handed back. - `RevertNativeForTransitionHeavyStages` gains `applyToAllStages`, because the preview holds a whole plan where AQE would have handed that rule one stage at a time. - Metadata-only statements (`CREATE VIEW`, `SHOW TABLES`) are not reported; a session runs enough of them that one 0% report each would bury the rest. - Nothing in the reporting path can fail the query: it runs off the query thread and swallows non-fatal failures with a warning. Measured in non-comment code lines the two approaches are the same size (123 vs 122 in main sources); the state machine's cost was mostly the prose needed to explain it. What differs is where the remaining complexity sits. Here it is one function that normalizes a plan, and when it gets a shape wrong the symptom is a coverage number that disagrees with the real plan — which a test catches mechanically. In #5394 a wrong guard shows up as a missing or duplicated report, which nothing catches until someone reads the log. ## How are these changes tested? New `CometPlanOnlySuite`, 20 tests. Two properties: - **The query runs on Spark.** V1 and V2 scans, AQE on and off, asserting the executed plan holds no `CometPlan` operator and that the intended scan path was actually exercised. - **The report describes what Comet would really have executed.** The same query is run with Comet enabled and the report's coverage compared against `CometCoverageStats` for the plan that ran, for an aggregate, a shuffled join, a scalar subquery, a DPP subquery and a stage forced through transition reversion. AQE on and off, except DPP (non-AQE only, because #5486 still breaks that combination). Plus: one report per action for a multi-stage query with a subquery, two actions reported twice, an RDD action reported once, an adaptive query that collapses to an empty relation reported once, metadata-only statements not reported, and the config off leaving Comet in charge. Two behaviours found while writing those tests are documented rather than fixed: - Under AQE the report describes the plan AQE settled on, which is the plan Comet would have been asked to run. If a stage materialized empty and AQE replaced the query with an empty relation, that is what gets reported. - Under AQE the transition count can be one lower than a real Comet run's, because Spark inserts transitions one stage at a time while the report is a single pass over the flattened plan. The operator counts behind the percentage are unaffected, and the tests compare transitions with AQE off. The estimate remains Scala-side only — the plan is never handed to DataFusion, so a DataFusion planning failure still counts as accelerated. That is called out in the config docstring and the user guide. `CometPlanOnlySuite`, `CometExecRuleSuite`, `CometScanRuleSuite`, `RevertNativeForTransitionHeavyStagesSuite` and `CometCoverageStatsSuite` are green (64 tests) on the default profile, `test-compile` is clean on `spark-3.4`, `spark-3.5`, `spark-4.0` and `spark-4.2`, and scalafix passes with the semantic rules. -- 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]
