andygrove commented on PR #5394: URL: https://github.com/apache/datafusion-comet/pull/5394#issuecomment-5432403306
All three fixed in c75a87c. The DPP one was as you described. `PlanDynamicPruningFilters` prepares the build plan and wraps it in the exchange afterwards, so the preparation boundary — and the stage `RevertNativeForTransitionHeavyStages` judged — is the exchange's child. The preview now descends through the wrapper and puts it back, and on your configuration the outer report reads 2/12 with 3 transitions, matching `CometCoverageStats` for the executed plan exactly. There's a test that builds a partitioned fact table joined to a filtered dimension with reversion forced on and asserts that equality. The empty adaptive plan I reproduced too, and it turned out the hash was not the only guard that missed: the mark could not reach the new tree either, since `AQEPropagateEmptyRelation` keeps the plan it replaced in `EmptyRelationExec`'s `innerChildren` rather than its children. Instead of chasing it through there I went with a third guard: once AQE has cut a query stage for an execution, everything that arrives afterwards is AQE re-planning something already reported, so the rest of the execution is suppressed. That is ordering-safe for subqueries because they are all compiled before the first stage is cut. The exception is a subquery AQE only plans once stages are under way — a DPP subquery, since `PlanAdaptiveDynamicPruningFilters` is a stage optimizer rule — which now gets no report of its own under AQE. That seemed a better trade than an extra bogus 0% report, but say the word if you'd rather keep it. Noted in the user guide, and tested under AQE on and off. You were right about the V2 tests, and the cause is that `withParquetTable` resolves the relation through `spark.read` and registers the result as a temp view, so the source list is baked in at creation. The config now wraps the fixture, and each variant asserts it actually got a `FileSourceScanExec` or a `BatchScanExec` so it cannot silently stop covering the path again. Separately, while building the DPP fixture I hit an unrelated pre-existing bug and filed #5486: with AQE on, DPP in play, and `spark.comet.exec.transitionRevert.enabled=true`, the query throws `SubqueryAdaptiveBroadcastExec does not support the execute() code path`. It reproduces on this PR's base commit with plan-only mode never enabled, and only when `transitionRevert` is on — so it's out of scope here, but it matters for #5207. It's why the new DPP test is non-AQE only, beyond the report being pre-adaptive anyway. `CometExecRuleSuite` passes on Spark 3.4 (42, 2 pre-existing version-guard cancellations), 3.5 (43, 1) and 4.1 (44), and `RevertNativeForTransitionHeavyStagesSuite`, `CometScanRuleSuite`, `CometCoverageStatsSuite` and `CometDppFallbackRepro3949Suite` are green. -- 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]
