Skip to content

chore: deprecate Spark 3.4 rather than removing it in 1.1.0 - #5885

Open
andygrove wants to merge 3 commits into
apache:mainfrom
andygrove:spark-sql-support-update-27ab8f26
Open

chore: deprecate Spark 3.4 rather than removing it in 1.1.0#5885
andygrove wants to merge 3 commits into
apache:mainfrom
andygrove:spark-sql-support-update-27ab8f26

Conversation

@andygrove

@andygrove andygrove commented Sep 12, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Part of #4329. This PR does not close it — removal is deferred, not cancelled, so #4329 stays open to track the eventual drop. It will need its 1.1.0 milestone moved.

Rationale for this change

Spark 3.4 was slated for removal in 1.1.0, and the docs said so in three places. We got pushback on that timing, so this PR walks the commitment back: Spark 3.4 becomes deprecated with no scheduled removal date, and we keep building and publishing its binaries.

What deprecation buys us is CI time. The Spark 3.4 Spark SQL suite currently runs in the merge queue on every merge, where it is one of the slowest jobs and a flake in it blocks everyone. Since we are no longer committing to fix Spark 3.4 regressions with the same urgency, it does not need to gate merges.

It does still need to be runnable. Anyone working on spark/src/main/spark-3.4/ or dev/diffs/3.4.3.diff should be able to get a verdict before merging, and run-spark-3.4-tests already exists for exactly that.

What changes are included in this PR?

CI. spark_3_4 in dev/ci/compute-changes.py goes from ["queue", "label:run-spark-3.4-tests"] to ["label:run-spark-3.4-tests"], which drops it from the merge queue while leaving the label and workflow_dispatch paths intact:

Event spark_3_4
merge_group false
PR, no label false
PR with run-spark-3.4-tests true
labeled + run-spark-3.4-tests true
workflow_dispatch true

Nothing else moves tier — 3.5, 4.0, 4.1 and all four Iceberg jobs still run in the queue.

spark_3_4 stays in required_checks.needs, but @sunchao was right that this does not make a
labelled run a gate, and the first version of this PR claimed it did. Adding the label fires a
labeled event, and on that event the aggregator deliberately publishes Required Checks (label run) rather than the Required Checks context .asf.yaml requires, so a red 3.4 there leaves an
earlier green required verdict alone. With 3.4 out of the queue there is no later run to catch it
either. 3.4 only counts toward the required verdict on a pull_request event that is not
labeled -- a push with the label already applied, or a PR opened with it:

Event spark_3_4 runs Aggregator name Gates the merge
PR opened / pushed, no label no Required Checks n/a
run-spark-3.4-tests just added yes Required Checks (label run) no, advisory
push with the label retained yes Required Checks yes
PR opened with the label yes Required Checks yes
merge_group no Required Checks n/a

Making the label run itself required is not an option: on labeled the PR tier is deliberately
skipped, so publishing that verdict as Required Checks would let it overwrite the commit run and
reintroduce #5007. So the fix is to say what actually happens. The spark_3_4 job comment, the
workflows README, contributor-guide/ci.md and contributor-guide/spark-sql-tests.md now state
that the label run is advisory, that the next push is what makes 3.4 blocking, and that with no
queue run behind it the label run's result has to actually be read before merging.

Docs.

  • compatibility/spark-versions.md, installation.md: removal target changes from "the 1.1.0 release" to "a future release", and both now say the Spark SQL suite no longer runs on every change, so a Spark 3.4 regression is more likely to reach a release. The installation CI table reads On demand for 3.4's Spark SQL column. The JDK 11 deprecation was sharing a sentence with Spark 3.4 — I split them so its 1.1.0 removal is unaffected.
  • about/versioning_policy.md: the lifecycle said removal happens in "the following" Comet minor release, and gave Spark 3.4 / 1.1.0 as the worked example. Reworded to "a later Comet minor release", removal not scheduled in advance, with "stops gating merges on the Spark SQL suite" folded into what deprecation means. This is the one edit that changes policy rather than just describing the new state — happy to instead keep the next-minor promise and write 3.4 up as a stated exception, if reviewers prefer that.
  • contributor-guide/ci.md, .github/workflows/README.md, ci.yml header comments: 3.4 moves out of the queue-tier listings into its own "label or dispatch only" row, with guidance to reach for the label deliberately since nothing else will run it.
  • contributor-guide/spark-sql-tests.md: the "Running Tests in CI" section claimed opening a PR was enough and pointed at spark_sql_test.yaml / spark_sql_test_ansi.yaml, which no longer exist. Rewritten with the per-version reality and the gh pr edit --add-label command.

How are these changes tested?

dev/ci/check-ci-config.py is the test for the routing policy, and it needed updating in a way that is itself the coverage. spark_3_4 moves out of SPARK_OPT_IN into its own SPARK_DEPRECATED set, which keeps it in ALL_JOBS (so workflow_dispatch still asserts it runs) while taking it out of QUEUE_TIER (so the merge_group case now asserts its absence rather than accepting it coming back silently). Added a labeled + run-spark-3.4-tests case to pin that the manual path stays reachable.

Locally green: check-ci-config.py, check-suites.py, test-iceberg-shards.py, apache-rat:check, actionlint --shellcheck=off, and prettier --check on every file this PR touches.

I also drove compute-changes.py directly for each event shape to produce both tables above, pairing each result with the aggregator name that event publishes, rather than trusting the assertions alone.

Three markdown files fail prettier --check at this branch's base (.ai/skills/review-comet-pr/SKILL.md, contributor-guide/debugging.md, contributor-guide/native_shuffle.md). Pre-existing and untouched here.

apache/main is merged in as of f69c4c81b. The conflicts were all with #5871, which moved the Spark 4.1 sql_hive shards behind the queue and so touched the same tier listings this PR edits: the ci.yml and README queue-tier prose, the README ASCII diagram and routing table, and SPARK_OPT_IN / the POLICY_CASES list in check-ci-config.py. Resolved by keeping both changes in each -- spark_4_1_hive added to the queue tier, spark_3_4 taken out of it.

Spark 3.4 was slated for removal in the 1.1.0 release, but there is still
enough usage that removing it now is premature. Keep building and publishing
Spark 3.4 binaries, and treat the version as deprecated with no scheduled
removal date instead.

Drop the Spark 3.4 Spark SQL suite from the merge queue, so it no longer gates
a merge. It stays fully runnable on demand: the `run-spark-3.4-tests` label on
a pull request, or a manual `workflow_dispatch` of `ci.yml`.

Update the docs to match: the removal target, what deprecation now implies for
test coverage, and the CI tier tables and diagrams.
@github-actions github-actions Bot added the enhancement New feature or request label Sep 12, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 13ae6e45f653d54735e630026619b2508af12caf.

The change consistently defers Spark 3.4 removal, preserves its Comet test/build and release-publishing paths, and removes only its automatic merge-queue Spark SQL coverage. Other supported Spark versions and the Iceberg queue policy are unchanged.

I found one issue in the new claim that applying the Spark 3.4 label gates the PR. The label-triggered run reports a separate, non-required status, and this change removes the later queue coverage. Details and a suggested correction are inline. No other actionable findings.

Validation:

  • check-ci-config.py passed, covering 7 path cases and 16 event-policy cases. Independently executed the base/head routing scripts across unlabeled PR, label-added, labeled synchronization, merge-group, and manual-dispatch scenarios. Only the merge-group Spark 3.4 output changed.
  • Suite-inventory validation and the remaining Python/Node preflight helpers passed, including all 15 Iceberg shard tests and 4 PR-label tests.
  • Current CI run: 57 successful checks and 10 skipped, with no failed or pending checks. The four Spark 3.4 Comet test shards passed. Its separate upstream Spark SQL suite was skipped.

Local validation covered routing scripts and workflow/configuration semantics. I did not launch a hosted failing label run or rebuild/run the native and JVM suites locally for this CI/documentation change.

Comment thread .github/workflows/ci.yml Outdated
Comment on lines +261 to +262
# manual workflow_dispatch. It stays in `required_checks.needs` so that a
# labelled run still gates the pull request it was asked for.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Correct the promise that applying the label gates the PR

Could we correct this guarantee here and in the contributor docs? The documented gh pr edit --add-label run-spark-3.4-tests operation fires a labeled event. On that event, required_checks.name deliberately publishes Required Checks (label run), while .asf.yaml names only Required Checks as required. Keeping spark_3_4 in needs therefore propagates a failure or cancellation to the non-required label status without changing an earlier green required verdict. This PR also removes the subsequent Spark 3.4 queue run, so that later check no longer catches the failure.

I verified the base/head routing scripts and aggregator configuration locally. A subsequent push with the label retained does include Spark 3.4 in the normal Required Checks verdict. Please document that applying the label alone produces an advisory result, and explain the subsequent push needed for the required gate, or implement the gating promised by the current wording.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, and you're right — I had the mechanism backwards. Adding the label fires labeled, the aggregator publishes Required Checks (label run) on that event, and .asf.yaml only requires Required Checks, so keeping spark_3_4 in needs does nothing for the label run's own verdict. Dropping the queue run means nothing picks it up afterwards either.

I looked at implementing the gating instead and don't think it's available to us. On labeled the PR tier is deliberately skipped because it already ran at that commit, so publishing that run as Required Checks would overwrite the commit's real verdict with one that tested a single suite — exactly #5007. So I've gone with documenting it.

Driving compute-changes.py for each event shape and pairing it with the name the aggregator publishes:

Event spark_3_4 runs Aggregator name Gates the merge
PR opened / pushed, no label no Required Checks n/a
run-spark-3.4-tests just added yes Required Checks (label run) no, advisory
push with the label retained yes Required Checks yes
PR opened with the label yes Required Checks yes
merge_group no Required Checks n/a

ac7405d3b replaces the gating promise in the spark_3_4 job comment, the workflows README, contributor-guide/ci.md and contributor-guide/spark-sql-tests.md. Each now says the label run is advisory, that the next push with the label retained is what puts 3.4 into the required verdict, and — since there's no queue run behind it — that the label run's result has to actually be read before merging rather than relied on to block. I added the caveat to spark-sql-tests.md too, since that's where the gh pr edit --add-label command you quoted lives.

Also merged apache/main in. The conflicts were all with #5871, which moved the Spark 4.1 sql_hive shards behind the queue and so edits the same tier listings; resolved by keeping both moves.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completed the follow-up review of 13ae6e45 against base de1eb4f8. The existing P2 about label-run gating remains: applying run-spark-3.4-tests reports the advisory Required Checks (label run) status. A subsequent push with the label retained includes the suite in the normal required verdict. I found no additional P1/P2 issue.

The CI configuration and suite-inventory checks pass locally. An independent base/head comparison across 36 event/path combinations confirms that only Spark 3.4's merge-queue routing changes. These checks validate routing and configuration, not a hosted failing label run.

At 2026-09-12 22:35:20 UTC, CI has 57 successful and 10 skipped checks. Preflight tested merge 8efc606a, whose parents are exactly this base and head, and all nine changed files match the reviewed head. Required Checks passed with Spark 3.4's upstream SQL suite skipped.

…ate-27ab8f26

# Conflicts:
#	.github/workflows/README.md
#	.github/workflows/ci.yml
#	dev/ci/check-ci-config.py
…uest

Adding `run-spark-3.4-tests` fires a `labeled` event, and on that event the
aggregator deliberately publishes `Required Checks (label run)` rather than the
`Required Checks` context `.asf.yaml` requires. A red Spark 3.4 in a label run
therefore leaves an earlier green required verdict in place, and with 3.4 no
longer in the merge queue there is no later run to catch it either.

Spark 3.4 only counts toward the required verdict on a pull_request event that
is not `labeled` -- a push with the label already applied. Say so in the
`spark_3_4` job comment, the workflows README, the CI guide and the Spark SQL
tests guide, rather than promising a gate the label alone does not provide.
@andygrove
andygrove force-pushed the spark-sql-support-update-27ab8f26 branch from ac7405d to cae1b46 Compare September 13, 2026 14:35

@rich7420 rich7420 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andygrove thanks for the patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants