sunchao commented on code in PR #5394:
URL: https://github.com/apache/datafusion-comet/pull/5394#discussion_r3813159766
##########
spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala:
##########
@@ -573,7 +595,17 @@ case class CometExecRule(session: SparkSession)
newPlan
}
- private def _apply(plan: SparkPlan): SparkPlan = {
+ /**
+ * Build the Comet plan we would have executed and log it. Called from
`_apply` in plan-only
+ * mode; the built plan is discarded. Passes `forPreview = true` through the
nested calls so
+ * both rules run their normal transforms instead of short-circuiting.
+ */
+ private def reportPlanOnlyCoverage(plan: SparkPlan): Unit = {
+ val preview = _apply(CometScanRule(session)._apply(plan), forPreview =
true)
Review Comment:
[P2] Include post-columnar reversion in the coverage preview
Could the preview run transition insertion and Comet's post-columnar rules
before computing coverage? Normal planning subsequently runs
`RevertNativeForTransitionHeavyStages` and `EliminateRedundantTransitions`, but
this path stops before them. With
`spark.comet.exec.transitionRevert.enabled=true`,
`spark.comet.exec.transitionRevert.maxTransitions=0`, and Comet project
execution disabled, the [existing regression
case](https://github.com/apache/datafusion-comet/blob/eb514de1ff76785b81a4940d6a2adb3ea149ab07/spark/src/test/scala/org/apache/comet/rules/RevertNativeForTransitionHeavyStagesSuite.scala#L196-L220)
shows that the real executed plan has zero `CometExec` nodes. This preview
still counts the operators that the configured Scala-side rule removes, and its
transition count is calculated before Spark inserts those transitions. That is
separate from the documented uncertainty about DataFusion planning failures.
Please compare the report with the real post-columnar plan for this c
onfiguration.
##########
spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala:
##########
@@ -589,6 +621,20 @@ case class CometExecRule(session: SparkSession)
plan
}
} else {
+ // Plan-only mode: build the Comet plan Comet would have executed, log
it, and return
+ // the original plan unchanged. `CometScanRule` also short-circuits in
this mode, so
+ // `plan` is still pure Spark; `reportPlanOnlyCoverage` rebuilds a
scan-wrapped copy for
+ // the preview. Placed before
`normalizePlan`/`RewriteJoin`/`tagUnsafePartialAggregates`
+ // so their work is not wasted on the discarded outer pass.
+ if (!forPreview && CometConf.COMET_EXPLAIN_PLAN_ONLY_ENABLED.get()) {
+ val executionId = Option(
+ session.sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY))
+ if (CometExecRule.markPlanOnlyReported(executionId)) {
Review Comment:
[P2] Keep the report slot for the outer query
Could we avoid consuming the execution-ID entry while Spark is preparing a
nested subquery? For a fresh action on `SELECT id FROM range(10) WHERE id >
(SELECT max(id) FROM range(3))`, Spark prepares the scalar subquery before the
outer plan under the same execution ID. The subquery records the ID here, so
the later outer-query call is suppressed. I reproduced this planning order with
the same deduplication logic on Spark 3.5.2 with AQE both disabled and enabled.
The only report describes `max(id)`, not the outer scan/filter or the workload
being evaluated. Please distinguish root-query reporting from subquery/stage
preparation and add an action-based test that captures the warning and checks
that the outer plan appears.
--
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]
