Enable SPARK-57298 collect_set tests when Spark 4.2 SQL diff lands - #5228
Draft
peterxcli wants to merge 20 commits into
Draft
Enable SPARK-57298 collect_set tests when Spark 4.2 SQL diff lands#5228peterxcli wants to merge 20 commits into
peterxcli wants to merge 20 commits into
Conversation
Adds dev/diffs/4.2.0-preview4.diff so the Spark SQL test suite can run against Apache Spark 4.2.0-preview4 with Comet enabled, and wires the 4.2 profile into the spark_sql_test workflow matrix. The diff was seeded from 4.1.1.diff and reconciled against v4.2.0-preview4.
Inherited from 4.1.1.diff during reconciliation. The Spark build was trying to resolve comet-spark-spark4.1_2.13 instead of the 4.2 artifact, causing the sql_hive jobs to fail before any tests ran.
Spark 4.2 removes GeographyVal / GeometryVal and replaces the getGeography and getGeometry accessors on SpecializedGetters with a single getBinaryView returning BinaryView. The shim therefore can no longer be shared between 4.1 and 4.2 out of the spark-4.1+ source root: the major, minor-plus and minor shim directories are all added as compile source roots, so a spark-4.2 copy would be a duplicate class rather than an override. Move the trait into per-version spark-4.1 and spark-4.2 copies.
Point spark.version at the released 4.2.0 instead of 4.2.0-preview4. Spark 4.2.0 bundles its own copy of org.apache.datasketches.memory.internal.ResourceImpl inside spark-catalyst, which collides with the transitive datasketches-memory jar and fails the maven-enforcer BanDuplicateClasses rule, so ignore that class. Keep the test-scope Jetty pin 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.
Reseed the Spark SQL test overrides from 4.1.2.diff against the v4.2.0 tag and drop the 4.2.0-preview4 diff. The notable reconciliation is the test harness refactor: Spark 4.2 shrinks SQLTestUtils to a deprecated empty alias and moves withSQLConf, stripSparkFilter and the test() override 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 has to qualify classic.SparkSession there, since a bare SparkSession in that package resolves to the unified class.
Regenerated with dev/regenerate-golden-files.sh --spark-version 4.2. The q2, q5, q54 (v1_4) and q5a (v2_7) goldens are pruned: their plans now match the shared fallback under the released 4.2.0, so the divergences seen on 4.2.0-preview4 are gone. q77a (v2_7) is added because Spark 4.2 plans a OneRowRelation into its Union branches. Comet cannot convert that leaf, so the Unions fall back and the aggregates above them lose their Comet partial producer. The approved plan records the degraded plan; see apache#4949.
Add a spark_4_2 job to the ci.yml umbrella calling spark_sql_test_reusable.yml with Spark 4.2.0 on JDK 17, plus the matching spark_4_2 filter in compute-changes.py and output on the changes job. Gate it behind the run-spark-4.2-tests label so it runs on pushes to main but stays off the default PR path while 4.2 support is experimental. Update the user guide for the released 4.2.0 and refresh the new-Spark-version contributor guide, which still described the spark_sql_test.yml matrix that was replaced by the per-version umbrella jobs.
The preflight job only proceeds on a labeled event for known gating labels. Without run-spark-4.2-tests in that allowlist, labelling a PR to request the Spark 4.2 SQL tests skipped the whole pipeline instead of running them.
Spark 4.2.0 is now in Maven Central, so the spark-4.2 profile no longer needs to target a preview. Point spark.version at 4.2.0 and make the main sources compile against it. - Drop the WIP compile-only comment on the spark-4.2 profile. - 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; ignore that class. - 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, so split it into per-version spark-4.1 and spark-4.2 copies. - Correct the stale Jetty comment that referenced preview4. This does not wire up the Spark SQL tests for 4.2; it only moves the profile onto the released version and keeps it compiling.
Regenerated with dev/regenerate-golden-files.sh --spark-version 4.2 against the released 4.2.0. The q2, q5, q54 (v1_4) and q5a (v2_7) goldens are pruned because their plans now match the shared fallback on 4.2.0, and q77a (v2_7) is added to record a degraded plan.
Two Spark 4.2.0 changes surfaced by moving the profile off preview4: Iceberg scans job: Spark 4.2.0 turned `connector.catalog.View` from an interface into a class. No Iceberg spark-runtime is published for 4.2, so the build reuses the 4.0 runtime, whose `SparkView implements View` now throws IncompatibleClassChangeError at class-load and aborts the Iceberg suites. Report Iceberg as unavailable on Spark 4.2 in the shared probes so the suites skip. Guard the fuzz suite's beforeAll with an early return (cancelling from beforeAll aborts the suite) and add the missing `assume(icebergAvailable)` to one native-scan test. Expressions job: Spark 4.2 normalizes NaN / -0.0 for array_distinct and the array set operations by wrapping their inputs as `KnownFloatingPointNormalized(ArrayTransform(arr, x -> NormalizeNaNAndZero(x)))`. The serde only handled a scalar `NormalizeNaNAndZero` child and fell back. Since `KnownFloatingPointNormalized` is a runtime no-op tag, serialize any other child directly and let its serde (the ArrayTransform codegen dispatcher) carry the normalization, keeping these operations native.
# Conflicts: # spark/src/main/scala/org/apache/comet/serde/contraintExpressions.scala
CometNativeScan serializes the full data and partition schema, not just the required columns. A table with a GEOMETRY or GEOGRAPHY column (Spark 4.2) has no proto representation for those types, so schema2Proto threw NoSuchElementException during planning instead of falling back. Reject the native scan when any data or partition schema field type cannot be serialized.
…issues Handle new-in-4.2 Spark SQL test suites that assert on Spark-internal execution mechanics Comet replaces wholesale, and repoint deferred assumes at specific issues instead of the closed generic tracker (apache#4142). - SLAM metric suites, segment-tree window suites, and the UnionExec whole-stage-codegen test are marked IgnoreComet / IgnoreCometSuite (apache#4963, apache#4964, apache#4965). - collect_set NaN/-0.0 normalization deferred (apache#4966). - Repoint assume(!isSpark42Plus) guards to specific issues: apache#4967 (ANSI arithmetic), apache#4968 (BloomFilter), apache#4969 (Iceberg REST catalog). - Geometry/geography scan tests now pass via native-scan fallback.
CodeQL flagged ci.yml as not limiting the GITHUB_TOKEN, so it inherits whatever the repository default is. Adds a top-level `permissions: contents: read`, matching the convention already used by codeql.yml and pyarrow_udf_test.yml. A blanket read-only default is not quite safe here, because ci.yml calls six reusable workflows and a callee inherits the caller's permissions and can only narrow them further. Audited all six: docs.yaml commits and pushes the generated site to the asf-site branch, so it needs write; the other five (pr_build_linux, pr_build_macos, pr_benchmark_check, spark_sql_test_reusable, iceberg_spark_test_reusable) only build and test. So the `docs` job raises itself to `contents: write` and nothing else changes. That job already only runs on push-to-main or workflow_dispatch, never on pull requests. Also checked what else might quietly depend on write access: no job in ci.yml or in any callee uses GITHUB_TOKEN, secrets, the gh CLI, git push (other than docs.yaml), containers or packages. The upload/download-artifact steps are all same-run, which uses the Actions runtime token rather than GITHUB_TOKEN, and none pass run-id or github-token, so none need `actions: read`. Verified the file parses and the blocks land where intended (top-level contents: read, docs job contents: write, no other job overriding). actionlint could not be run locally -- its installer is a curl-to-shell script -- so it will be exercised by the preflight job on this PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #5209.
Stacked on #4950 and should be merged after it. This also relies on the native
collect_setnormalization from #5166.Rationale for this change
Spark 4.2 includes SPARK-57298 coverage for normalizing NaN and signed zero in
collect_set. Comet previously ignored the scalar test because nativecollect_setdid not implement those semantics.What changes are included in this PR?
Regenerates
dev/diffs/4.2.0.diffwithout the SPARK-57298IgnoreCometmarker. The nested complex-type test was already enabled, so both upstream tests now run without adding another diff file.How are these changes tested?
DataFrameAggregateSuitetests: 2 passed, 0 failed, 0 ignored.CometSqlFileTestSuite collect_set_normalizationwith Spark 4.2 against fix: normalize floating-point values in native collect_set #5166: 2 passed for Parquet dictionary encoding on and off.git diff --check.