andygrove opened a new pull request, #4950: URL: https://github.com/apache/datafusion-comet/pull/4950
## Which issue does this PR close? Part of #4142. Supersedes #4208, which was opened against `4.2.0-preview4` and went stale. ## Rationale for this change Spark 4.2.0 is now available in Maven Central, so the `spark-4.2` profile no longer has to target a preview. This is stage 3 of the bring-up described in `adding_a_new_spark_version.md`: turn on Spark's own SQL tests for 4.2 and move the profile onto the released version. Spark 4.2.0 is not simply `4.2.0-preview4` with the qualifier dropped. It restructures the test harness, changes `SpecializedGetters`, and repackages a dependency, so several real reconciliations were needed. ## What changes are included in this PR? **Build** - `spark.version` moves from `4.2.0-preview4` to `4.2.0`. - Spark 4.2.0 bundles its own copy of `org.apache.datasketches.memory.internal.ResourceImpl` inside `spark-catalyst`, colliding with the transitive `datasketches-memory` jar and failing the `BanDuplicateClasses` enforcer rule. That class is now ignored. - The test-scope Jetty pin stays at 11.0.26. Spark 4.2.0 ships Jetty 12, but the Iceberg REST catalog test helper needs `jetty-servlet`, which Jetty 12 replaced with `jetty-ee10-servlet`. The stale comment claiming the pin matched preview4's Jetty version is corrected. **Shims** - Spark 4.2 removes `GeographyVal` / `GeometryVal` and replaces `getGeography` / `getGeometry` on `SpecializedGetters` with a single `getBinaryView` returning `BinaryView`. `CometInternalRowShim` can no longer be shared from the `spark-4.1+` source root, because the major, minor-plus and minor shim directories are all compile source roots (a `spark-4.2` copy would be a duplicate class, not an override). It is split into per-version `spark-4.1` and `spark-4.2` copies. **Spark SQL test diff** - `dev/diffs/4.2.0.diff` replaces `dev/diffs/4.2.0-preview4.diff`, reseeded from `4.1.2.diff` against the `v4.2.0` tag. - The main reconciliation is upstream's test-harness refactor: `SQLTestUtils` shrinks to a deprecated empty alias, and `withSQLConf`, `stripSparkFilter` and the `test()` override move into `QueryTest`, with `SharedSparkSession` now extending `QueryTest` directly. Comet's hooks (`isCometEnabled`, the `IgnoreComet` skip, and the `stripSparkFilter` Comet cases) move to `QueryTest` accordingly. `isCometEnabled` must qualify `classic.SparkSession`, since a bare `SparkSession` in that package resolves to the unified class. - `KeyGroupedPartitioningSuite` needs no Comet change any more: 4.2 already declares `collectAllShuffles` / `collectShuffles` as `protected` returning `Seq[ShuffleExchangeLike]`. - `HiveUDFDynamicLoadSuite` drops Comet's commented-out `assume`: 4.2 adds `TestHiveUdfsJar`, which builds `hive-test-udfs.jar` from Java sources at runtime, so the stripped-jar workaround is obsolete. **CI** - Adds a `spark_4_2` job to the `ci.yml` umbrella calling `spark_sql_test_reusable.yml` (Spark 4.2.0, JDK 17), plus the matching `spark_4_2` filter in `compute-changes.py` and output on the `changes` job. It is gated behind a `run-spark-4.2-tests` label so it runs on pushes to `main` but stays off the default PR path while 4.2 is experimental. **Plan stability** - Regenerated with `dev/regenerate-golden-files.sh --spark-version 4.2`. The `q2`, `q5`, `q54` (v1_4) and `q5a` (v2_7) goldens are pruned because their plans now match the shared fallback on the released 4.2.0, so the preview4-era divergences are gone. - `q77a` (v2_7) is added and records a degraded plan: Spark 4.2 plans a `OneRowRelation` into its `Union` branches, which Comet cannot convert, so the `Union`s fall back and the aggregates above lose their Comet partial producer. Tracked in #4949 and to be removed when that is fixed. **Docs** - User guide lists 4.2.0 (rather than preview4) in the experimental table, with Comet and Spark SQL tests now running in CI. - The new-Spark-version contributor guide described the `spark_sql_test.yml` matrix, which has since been replaced by the per-version umbrella jobs; it now documents the `ci.yml` + `compute-changes.py` wiring. ## How are these changes tested? - `./mvnw -Pspark-4.2 install` against the released 4.2.0 builds clean, as does `-Pspark-4.1`, confirming the shim split does not regress 4.1. - `dev/diffs/4.2.0.diff` was verified to apply with no rejects to a pristine `v4.2.0` checkout, and patches the same 95 files as `4.1.2.diff`, differing only by the expected `SQLTestUtils` to `QueryTest` swap. - `CometTPCDSV1_4_PlanStabilitySuite` and `CometTPCDSV2_7_PlanStabilitySuite` pass under `-Pspark-4.2` (103 and 32 tests, no failures). - The new `spark_4_2` job exercises the diff in CI; the `run-spark-4.2-tests` label is applied to this PR. -- 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]
