comphead opened a new pull request, #6577: URL: https://github.com/apache/datafusion-comet/pull/6577
## Which issue does this PR close? Closes #6463. ## Rationale for this change Spark builds the SQL tab's graph, and the plans the event log records, with `SparkPlanInfo.fromSparkPlan`. It gives its own `InMemoryTableScanExec` the cached plan as a child, but recognizes that scan by its class. For any other node it takes `plan.children ++ plan.subqueries`, so for a relation cached in Comet's format the tree ended at `CometInMemoryTableScan`, and the cached plan's metrics were missing below it. The issue expected that Comet could not fix this alone, because a child would make the cached plan part of the query that reads the cache. A subquery does not: Spark runs subqueries from a plan's expressions and only walks the `subqueries` list. ## What changes are included in this PR? This PR is stacked on #6574. The first commit is that PR's, so review the second one. It needs #6574's explicit `innerChildren` override: `QueryPlan.innerChildren` defaults to `subqueries`, so without it the cached plan would also land in EXPLAIN without its `InMemoryRelation` line, and in the coverage and fallback reporting of `ExtendedExplainInfo`. - `CometInMemoryTableScanExec` overrides `subqueries` to return the cached plan. It is a `lazy val` because Spark 3.x declares `subqueries` as one, and a `lazy val` overrides Spark 4's `def` as well. - A test-only `CometSparkPlanInfoHelper`, because the `SparkPlanInfo` companion object is `private[execution]`. Other readers of a physical plan's `subqueries` now follow it into the cached plan too, which they do not do for Spark's own scan. Checked against Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0: - `CollectMetricsExec.collect`, through `AdaptiveSparkPlanHelper.collectWithSubqueries`, would find observed metrics in the cached plan, as it does below Spark's scan by matching its class. Nothing changes today, because #6421 keeps Spark's scan for any relation whose cached plan records observed metrics. - `debugCodegen` output includes the cached plan's codegen stages. - On Spark 3.4 and 3.5, `AdaptiveSparkPlanExec.finalPlanUpdate` posts one more plan update when the final plan contains the scan outside a query stage. - On Spark 4.2, `SQLLastAttemptAccumulator` walks the cached plan too. Its documentation already declares cached plans undefined behavior, and it bails out on Comet's shuffle exchanges anyway. `CacheManager.validateCachedEntryForTransaction` could register one more scan for a nested cache in a DSv2 transaction. ## How are these changes tested? A new test in `CometInMemoryCacheSuite`, with AQE off and on, compares the `SparkPlanInfo` subtree below the scan with the cached plan's own `SparkPlanInfo`. Without the `subqueries` override it fails, with no children below the scan. `CometInMemoryCacheSuite` passes locally on the default Spark 4.1 profile (61 tests). Other profiles were not run locally, so `run-all-spark-profiles` is applied for CI to compile and run the suites on them. Spark's `CachedTableSuite` test "SPARK-35332: Make cache plan disable configs configurable - check AQE", which #5634 skips under Comet, reads the cached plan from this tree and should now find it. Whether it passes was not checked. -- 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]
