andygrove opened a new issue, #6406: URL: https://github.com/apache/datafusion-comet/issues/6406
### Describe the bug Some of the Spark SQL and Iceberg tests that CI runs "with Comet" actually run on plain Spark, and they pass without anyone noticing. **Spark SQL.** About 20 `sql_core` suites per Spark version build their own `SparkSession` or `SparkContext` instead of going through `SharedSparkSession` or `TestHive`, so they never get the Comet shuffle manager. The diff's `SparkSession.applyExtensions` hook still injects `CometSparkSessionExtensions` into those sessions. But since #4328, `isCometLoaded` returns false when Comet shuffle is enabled and `CometShuffleManager` is not registered, so every query in them runs on Spark. The job logs show it as: ``` WARN org.apache.comet.CometSparkSessionExtensions: Comet extension is disabled because spark.shuffle.manager is not set to org.apache.spark.sql.comet.execution.shuffle.CometShuffleManager or org.apache.spark.sql.comet.execution.shuffle.CometCelebornShuffleManager. ... ``` That line appears about 20,400 times across the `sql_core` shards of the [09-29 nightly](https://github.com/apache/datafusion-comet/actions/runs/36531195399) (Spark 3.5 and 4.0) and the [merge queue run at 1628c5200](https://github.com/apache/datafusion-comet/actions/runs/36574699383) (Spark 4.1). Counting a test as affected when the warning is logged while it runs: | Suite | 3.5.9 | 4.0.4 | 4.1.3 | | --- | --- | --- | --- | | CoalesceShufflePartitionsSuite | 16/16 | 16/16 | 17/17 | | BroadcastJoinSuite | 16/19 | 16/19 | 16/19 | | BroadcastJoinSuiteAE | 15/19 | 15/19 | 15/19 | | SparkSessionExtensionSuite | 12/27 | 13/30 | 13/31 | | StateStoreCoordinatorSuite | 1/4 | 1/4 | 12/15 | | SQLExecutionSuite | 6/6 | 8/8 | 8/8 | | SparkSessionBuilderSuite | 7/35 | 6/36 | 6/36 | | ExecutorSideSQLConfSuite | 6/7 | 6/7 | 6/7 | | XmlPartitioningSuite | | 6/6 | 6/6 | | XmlPartitioningSuiteWithLegacyParser | | | 6/6 | | SQLContextSuite | 4/7 | 5/8 | 5/8 | | ParquetCommitterSuite | 4/4 | 4/5 | 4/5 | | UISeleniumSuite (execution.ui and streaming.ui) | 2/2 | 3/4 | 3/4 | | ExecutionListenerManagerSuite | 3/3 | 3/3 | 3/3 | | TransformWithStateClusterSuite | | 3/4 | | | SessionStateSuite | 2/7 | 2/7 | 2/7 | | StateStoreSuite | 1/87 | | | | UISeleniumWithRocksDBBackendSuite, BroadcastExchangeExecSparkSuite, BasicWriteJobStatsTrackerMetricSuite, SQLAppStatusListenerMemoryLeakSuite | 4/4 | 4/4 | 4/4 | | SparkSessionJobTaggingAndCancellationSuite | | | 1/5 | | MapStatusEndToEndSuite | | | 1/1 | | **Total** | **99** | **111** | **128** | The session-builder and UI suites don't matter much, but CoalesceShufflePartitionsSuite, BroadcastJoinSuite and SQLExecutionSuite are exactly the kind of coverage we want Comet to have. Everything else in the Spark SQL jobs does run Comet: there are no native library load failures or off-heap warnings, and native execution shows up in every `sql_core` shard. **Iceberg.** In Iceberg 1.11.0, `TestPartitionedWritesToWapBranch` moved from `spark` to `spark-extensions` and now stops the shared session and builds its own, without `spark.plugins=org.apache.spark.CometPlugin`. Its 36 tests run on plain Spark in the `iceberg-spark-extensions` job, and nothing warns, because without the plugin Comet is never loaded. Every other Iceberg test class that starts a `SparkContext` does load `CometDriverPlugin`. I checked that against the shard JUnit XML for 1.8.1 through 1.11.0, and with a scan of every `SparkSession.builder()` in the four Iceberg test trees. ### Steps to reproduce Grep the `Run Spark tests` step of any `spark-sql-sql_core-*` job for `Comet extension is disabled`. Attribute each line to the most recent `[info] <Suite>:` header. ### Expected behavior Every Spark SQL and Iceberg test that runs Spark queries runs them with Comet, unless the diff opts it out explicitly with `IgnoreComet` or `spark.comet.enabled=false`. ### Additional context Proposed fix: 1. In the Spark diffs, when Comet is enabled, pass `-Dspark.shuffle.manager=org.apache.spark.sql.comet.execution.shuffle.CometShuffleManager` to the forked test JVMs from `project/SparkBuild.scala`. That's how Spark itself sets test-wide defaults such as `spark.ui.enabled=false`. Every `SparkConf` that loads defaults picks it up, so suites that build their own sessions get Comet too, and a new suite can't regress silently. Tests that fail once Comet is actually running get Comet-aware assertions, or `IgnoreComet` with a reason. 2. In `dev/diffs/iceberg/1.11.0.diff`, add the Comet session configs to `TestPartitionedWritesToWapBranch`, as `ExtensionsTestBase` already has. Two shards can't exercise Comet by design and are out of scope here. `spark-sql-catalyst` never creates a `SparkSession`, and `spark-sql-sql_hive-2` runs only spark-submit and metastore-client tests. Spark 4.2 isn't covered by the numbers above, because its first nightly failed to build (fixed by #6398). -- 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]
