andygrove opened a new pull request, #6415: URL: https://github.com/apache/datafusion-comet/pull/6415
## Which issue does this PR close? Closes #6406. ## Rationale for this change Comet disables itself when its shuffle manager is not registered (#4328). About 20 `sql_core` suites per Spark version build their own `SparkSession` or `SparkContext` instead of going through `SharedSparkSession` or `TestHive`, so they never got the Comet shuffle manager and quietly ran on plain Spark: 99, 111 and 128 tests on Spark 3.5, 4.0 and 4.1. They include CoalesceShufflePartitionsSuite, BroadcastJoinSuite and SQLExecutionSuite. On the Iceberg side, 1.11's `TestPartitionedWritesToWapBranch` builds its own session without the Comet plugin. The issue has the evidence from the CI logs. ## What changes are included in this PR? Each Spark diff (3.4.3, 3.5.9, 4.0.4, 4.1.3, 4.2.0) adds a small `CometTestSettings` to `project/SparkBuild.scala`. When Comet is enabled and nobody has set `spark.shuffle.manager` explicitly, the `sql` and `hive` test JVMs get `-Dspark.shuffle.manager=...CometShuffleManager`. That's the same mechanism Spark's build already uses for test-wide defaults such as `spark.ui.enabled=false`. Every `SparkConf` that loads defaults picks it up, so a suite that builds its own session now runs Comet, and so will new suites. With Comet actually running, a few tests fail for reasons that are Spark-specific. They get the same adjustment on every version: - `CoalesceShufflePartitionsSuite`: AQE coalesces by map output size, and Comet's Arrow shuffle blocks differ in size from Spark's. So the exact coalesced-partition counts in three "determining the number of reducers" tests and the union test only hold for Spark's shuffle. Those checks are skipped under Comet, and the answer checks still run. - `BroadcastJoinSuite`: the `PartitioningCollection` test inspects the output partitioning of Spark's `SortMergeJoinExec` and `BroadcastHashJoinExec`, which Comet replaces. It is now `IgnoreComet`. - `SparkSessionExtensionSuite`: five tests inject columnar and AQE rules that never see the Spark operators they act on once Comet's rules take over. They're skipped when Comet is enabled. - `MapStatusEndToEndSuite` (4.1 and 4.2): Comet's shuffle doesn't report Spark 4.1's order-independent map output checksum. That is a real gap, filed as #6414, and the `IgnoreComet` reason points to it. `dev/diffs/iceberg/1.11.0.diff` gives `TestPartitionedWritesToWapBranch` the same Comet session configs as `ExtensionsTestBase`. The contributor guide's list of the diffs' changes to Spark now mentions the `SparkBuild.scala` default. A few tests build their context from `new SparkConf(false)`, which doesn't load system properties, so they still run without Comet. They are `ExecutionListenerManagerSuite` "SPARK-37780", `SQLContextSuite` on Spark 3.x (through core's `SharedSparkContext`), and one `StateStoreCoordinatorSuite` test. They only exercise session and listener plumbing, so I left them alone. `4.2.0.diff` keeps its current `comet.version` line, so this PR merges cleanly with #6398 in either order. ## How are these changes tested? Every diff was regenerated through the documented process. Each round-tripped cleanly before the change, and the new version applies cleanly to a pristine checkout of its tag. The change to each diff file only adds lines. Spark's sbt build can't resolve its dependencies on my machine, so I ran the affected suites for all five Spark versions from Spark's test jars under Comet's Maven build instead. I passed CI's forked-JVM options and `CometShuffleManager` as the default. The patched `SparkSession` hook, `IgnoreComet` and the `isCometEnabled` test harness were compiled in, so the edited suites ran as they would in CI. Without the test edits, the same 11 tests failed on every version, plus `MapStatusEndToEndSuite` on 4.1 and 4.2. With them there are no failures. No "Comet extension is disabled" warnings remain apart from the `SparkConf(false)` cases above. The UI Selenium suites and `XmlPartitioningSuite` can't run that way, so CI is the first run for those. For Iceberg, I ran `TestPartitionedWritesToWapBranch` against 1.11.0 locally: 36/36 passed, both of its SparkContexts loaded `CometDriverPlugin`, and all 76 of its writes went through Comet's native writer. The new default reaches every `sql` and `hive` suite that builds its own context, so the verdict comes from the `run-spark-3.4-tests`, `run-spark-3.5-tests`, `run-spark-4.0-tests` and `run-spark-4.1-tests` labels. Spark 4.2 can only be labelled after #6398 lands, and Iceberg 1.11 runs in the merge queue. -- 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]
