andygrove commented on PR #5394: URL: https://github.com/apache/datafusion-comet/pull/5394#issuecomment-5431259734
All three fixed in 1683a65. The Spark 3.x compile break was exactly as you described — the assertions now run inside the `withSQLConf` block rather than binding its result. `-Pspark-3.4` and `-Pspark-3.5` both `test-compile` clean here, and `CometExecRuleSuite` passes on 3.5 as well as on the default 4.1. On the no-execution-ID path: rather than scoping state to an ID that may not exist, the report is now marked on the plan itself. Catalyst copies tags onto replacement nodes, so the mark survives the rewrites Spark makes between one application of the rule and the next, which is what lets a later application recognise a stage or final-plan pass as a re-plan of something already reported. That turned out to matter beyond the no-ID case: I found a plan with no exchanges gets a final-plan pass under AQE whose hash no longer matches the prep-time plan, because the subquery was substituted in between, so the old dedupe let it through as a second report. I kept the execution-scoped hash alongside the mark for the one case a mark can't cover — a subquery referenced from two places is prepared twice as separate but identical plans. Both `df.rdd.count()` and `executedPlan` before an action are covered by tests now, under AQE on and off. The subquery one was worse than the double-count you found, and getting the outer report to agree with normal planning took two more things. A subquery reaches the outer plan fully prepared, so its aggregates are already inside `WholeStageCodegenExec` with transitions inserted, and handing that to the conversion pass leaves them unconverted — the preview was reporting a subquery as falling back that Comet actually accelerates. Those artifacts are stripped before previewing. And `ReuseExchangeAndSubquery` is the last thing `QueryExecution.preparations` does, after the columnar rules, so the plan the preview sees still has one copy of the subquery per reference; replaying that rule at the top level collapses them the way the executed plan does. With AQE off the outer report is now exactly `CometCoverageStats` for the plan Comet really executes — 6/7 with 2 transitions on your scalar-subquery query, against 6/12 with 5 transitions before — and there's a test asserting that equa lity. One thing I could not make match, and I'd like your read on it. Under AQE the outer report is produced from the prep-rule application, and at that point `PlanAdaptiveSubqueries` hasn't run, so the subquery is still a logical plan hanging off the expression. Extended explain walks it anyway and counts those logical nodes as un-accelerated Spark operators, so the same query reports 2/7 under AQE against 6/7 off. Reporting operators we never evaluated as fallbacks is the part that bothers me. The alternatives I can see are to exclude un-planned subqueries from the counts, which trades a pessimistic number for an optimistic one, or to report from the post-adaptive application instead of the prep one, which is only reachable for queries AQE never cuts into stages. For now it's documented, with a pointer to the per-subquery reports. Would you rather I pursue one of those, or is documenting it acceptable for a first cut? The remaining red check is `spark-sql-sql_hive-1`, which failed downloading `apache-maven-3.9.6-bin.zip` from Maven Central — unrelated to this PR. -- 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]
