comphead commented on code in PR #6577: URL: https://github.com/apache/datafusion-comet/pull/6577#discussion_r4178468779
########## spark/src/main/scala/org/apache/spark/sql/comet/CometInMemoryTableScanExec.scala: ########## @@ -80,6 +81,21 @@ case class CometInMemoryTableScanExec( // newlines, which breaks the tree of every plan that reads the cache. override def stringArgs: Iterator[Any] = Iterator(originalPlan) + // Spark's own scan lists its InMemoryRelation as an inner child, and the relation lists the + // cached plan, so EXPLAIN draws the plan that built the cache below the scan. Do the same. + // ExtendedExplainInfo leaves them out of Comet's own reporting: the cached plan runs when the + // relation is materialized, not as part of every query that reads it. + override def innerChildren: Seq[QueryPlan[_]] = Seq(originalPlan.relation) + + // SparkPlanInfo, which the SQL tab's graph and the event log's plans are built from, gives + // Spark's own scan its cached plan as a child, but recognizes that scan by its class. For any + // other node it takes the children and the subqueries, so expose the cached plan as the one + // subquery. A child would make the cached plan part of the query that reads the cache, but + // Spark only walks this list: subqueries run from a plan's expressions. Its other walkers, such + // as collectWithSubqueries, follow it into the cached plan too. A lazy val, because Spark 3.x + // declares subqueries as one, and a lazy val overrides Spark 4's def as well. + @transient override lazy val subqueries: Seq[SparkPlan] = Seq(originalPlan.relation.cachedPlan) Review Comment: Thanks, confirmed and fixed in c190f6e3ac (rebased onto main after #6574 merged). `subqueries` now returns Spark's own `InMemoryTableScanExec` (`originalPlan`) rather than the cached plan. `SparkPlanInfo` special-cases that class, so the SQL tab and event log still draw the cached plan, one level down, while `SQLLastAttemptAccumulator` and the other subquery walkers stop at that scan, as they do in Spark's own plans. Your outside-cache scenario is now a 4.2-only `CometInMemoryCacheLastAttemptMetricSuite`: it checks that the cached plan has a Comet shuffle, then expects `Some(100)`. A check on every version also asserts that `collectWithSubqueries` no longer reaches the cached plan's shuffle. The 4.2 suite could not run locally, so the `run-all-spark-profiles` run gives its first result. -- 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]
