andygrove opened a new pull request, #6195: URL: https://github.com/apache/datafusion-comet/pull/6195
## Which issue does this PR close? Closes #6186. ## Rationale for this change On-heap mode exists so the Spark SQL tests can run against Comet, and it has to be enabled with `spark.comet.exec.onHeap.enabled`. Only `CometDriverPlugin` enforced that. An application that registers `CometSparkSessionExtensions` directly with `spark.sql.extensions` and leaves off-heap memory disabled got Comet in on-heap mode with no warning. Today that puts each task's native memory in a fixed pool that Spark cannot see, and once #6066 lands, on-heap mode does no memory accounting at all. This change does not depend on #6066 and can land before it. ## What changes are included in this PR? - `isCometLoaded`, which `CometScanRule`, `CometExecRule` and `IcebergWriteStrategy` all check, now returns false unless `spark.memory.offHeap.enabled` or `spark.comet.exec.onHeap.enabled` is true, and logs the same warning as the plugin. Like the shuffle manager warning next to it, the warning is logged each time the check runs. - The check reads both settings from the session conf, so the `ENABLE_COMET_ONHEAP` environment variable still supplies the default for `spark.comet.exec.onHeap.enabled`. The Spark SQL and Iceberg CI jobs set it, so the Spark diffs need no change. The plugin keeps its own check, because it also skips the rest of its setup. - The benchmarks that register the extension without enabling off-heap memory now set `spark.comet.exec.onHeap.enabled=true`, so they keep running Comet instead of comparing Spark with Spark: `CometBenchmarkBase`, the six benchmarks that build their own session, and `CometTPCQueryBase`. - The IntelliJ steps in the Spark SQL tests guide now set `ENABLE_COMET_ONHEAP=true` alongside `ENABLE_COMET=true`, as the sbt commands on the same page already do. - The Kubernetes example in `benchmarks/README.md` registers the extension directly and had no off-heap settings, so it would have lost Comet. It now enables off-heap memory and moves 16G of the 32G executor memory into `spark.memory.offHeap.size`, so the executor pod requests the same total. ## How are these changes tested? - A new `CometSparkSessionExtensionsSuite` test covers `isCometLoaded` with neither setting, with off-heap memory only, and with `spark.comet.exec.onHeap.enabled` only. The existing `isCometLoaded` tests now start from a conf with off-heap memory enabled, so they still reach the checks they were written for. - A new `CometPluginsExtensionOnlySuite` reproduces the issue: the extension registered directly, the Comet shuffle manager, and neither the plugin nor off-heap memory. Before the fix the query planned `CometNativeScan` and `CometFilter`. Now it plans no Comet operators and logs the warning. A second test enables `spark.comet.exec.onHeap.enabled` for the session and gets Comet operators back. The suite is registered in both PR workflows. - `CometTPCDSV1_4_PlanStabilitySuite`, which registers the extension directly with off-heap memory enabled, still matches its approved plans. `CometPluginsSuite`, `CometPluginsDefaultSuite`, `CometPluginsUnifiedModeSuite` and `CometInMemoryCacheSuite` also pass locally. -- 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]
