andygrove opened a new issue, #6186: URL: https://github.com/apache/datafusion-comet/issues/6186
### Describe the bug On-heap mode exists so the Spark SQL tests can run against Comet, and it has to be switched on with `spark.comet.exec.onHeap.enabled`. The only thing that enforces this is `CometDriverPlugin.init`. When off-heap memory is disabled and on-heap mode is not enabled, it returns before registering the session extension ([Plugins.scala#L58-L62](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/spark/src/main/scala/org/apache/spark/Plugins.scala#L58-L62)). `CometSparkSessionExtensions.isCometLoaded`, which both `CometScanRule` and `CometExecRule` check, does not look at the memory mode ([CometSparkSessionExtensions.scala#L123](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala#L123)), and nothing else reads `spark.comet.exec.onHeap.enabled`. An application can register the extension directly with `spark.sql.extensions=org.apache.comet.CometSparkSessionExtensions`, as the [spark-sql-perf benchmarking guide](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/docs/source/contributor-guide/benchmarking_spark_sql_perf.md?plain=1#L240) does (that example does enable off-heap memory). If such an application leaves off-heap memory disabled, Comet runs in on-heap mode with no warning. Today that means each task's native memory comes from a fixed-size pool, carved out of `spark.comet.memoryOverhead` (1 GiB per executor by default), that Spark cannot see. Once #6066 lands, on-heap mode does no memory accounting at all, so native memory becomes unbounded. The plugin's other startup checks are skipped on this path too: the `spark.executor.memoryOverhead` warning and the `spark.comet.exec.memoryPool.fraction` deprecation warning. ### Steps to reproduce Start a session with `spark.sql.extensions=org.apache.comet.CometSparkSessionExtensions`, the Comet shuffle manager and `spark.comet.enabled=true`, with neither `spark.plugins` nor any off-heap settings. `isCometLoaded` returns true, the rules plan Comet operators, and `getMemoryConfig` takes the on-heap path. ### Expected behavior Comet stays disabled and logs the same warning the plugin does, unless off-heap memory is enabled or `spark.comet.exec.onHeap.enabled=true`. ### Additional context Suggested fix: move the check into `isCometLoaded`, landing with #6066 or before it. -- 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]
