andygrove opened a new issue, #5245:
URL: https://github.com/apache/datafusion-comet/issues/5245

   ### What is the problem the feature request solves?
   
   #5051 adds `CometInMemoryTableScanExec` and a Comet cache serializer 
(`CometCachedBatch`) behind `spark.comet.exec.inMemoryCache.enabled`. 
`CometInMemoryCacheSuite` covers the cache format, stats pruning, storage 
levels, and the fallback paths, but all of its tests run with AQE disabled.
   
   AQE interacts with cached relations through machinery that the suite never 
exercises: `TableCacheQueryStageExec` materializes a cached plan as a query 
stage, and AQE re-optimizes using the cache's runtime statistics. Comet's scan 
replaces the node those mechanisms wrap, so a regression there (wrong stage 
materialization, wrong row-count/size statistics driving a join strategy) would 
not be caught today.
   
   ### Describe the potential solution
   
   Port the AQE + table-cache scenarios from Spark's `AdaptiveQueryExecSuite` 
and run them with `spark.comet.exec.inMemoryCache.enabled=true`, asserting the 
cached scan is `CometInMemoryTableScanExec`:
   
   1. 
[SPARK-42101](https://github.com/apache/spark/blob/v4.1.2/sql/core/src/test/scala/org/apache/spark/sql/execution/adaptive/AdaptiveQueryExecSuite.scala#L3114-L3153)
 — leaves a cached join cold, first-touches it through an AQE aggregation, and 
checks cold/warm materialization and the resulting plan rewrites.
   2. [Table-cache stage in an AQE 
join](https://github.com/apache/spark/blob/v4.1.2/sql/core/src/test/scala/org/apache/spark/sql/execution/adaptive/AdaptiveQueryExecSuite.scala#L3156-L3190)
 — verifies `TableCacheQueryStageExec` and the surrounding shuffle behavior.
   3. 
[SPARK-37742](https://github.com/apache/spark/blob/v4.1.2/sql/core/src/test/scala/org/apache/spark/sql/execution/adaptive/AdaptiveQueryExecSuite.scala#L2780-L2831)
 — verifies AQE does not choose a join strategy from invalid cache runtime 
statistics.
   
   Scenario 3 is worth particular attention: the Comet serializer reports its 
own sizes for `CometCachedBatch`, so the statistics AQE reads come from Comet 
code rather than Spark's `DefaultCachedBatch` accounting.
   
   ### Additional context
   
   Deferred from review of #5051 
(https://github.com/apache/datafusion-comet/pull/5051#discussion_r3682179422). 
The cache path itself is off by default, so this is test coverage for an 
experimental feature rather than a fix for known-broken behavior.
   


-- 
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]

Reply via email to