Skip to content

ci: drop non-existent suites from the Linux and macOS test matrices - #6099

Merged
andygrove merged 4 commits into
apache:mainfrom
ErikBPF:ci/drop-nonexistent-suites
Oct 5, 2026
Merged

andygrove merged 4 commits into
apache:mainfrom
ErikBPF:ci/drop-nonexistent-suites

Conversation

@ErikBPF

@ErikBPF ErikBPF commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

None. Small CI-hygiene follow-up; no tracking issue.

Rationale for this change

dev/ci/check-suites.py only verified that every suite in the source tree is
listed in a workflow. It never verified the reverse, so six suite names that no
longer exist remained in the linux-test and macOS matrices:

CI is green only because the suites are passed via -DwildcardSuites, which
ignores unmatched names; an explicit -Dsuites list fails with
ClassNotFoundException. The matrices therefore claim coverage that does not
run, and nothing prevented a stale name from being re-added.

What changes are included in this PR?

  • Add a forward declaration check to dev/ci/check-suites.py: a suite listed in
    a workflow must be declared as a class/trait/object in some sourceset.
  • Remove the six dead suite lines from .github/workflows/pr_build_linux.yml
    and .github/workflows/pr_build_macos.yml.

ParquetReadV1Suite and CometStringDecodeSuite stay listed: the former is
declared in ParquetReadSuite.scala, the latter in the spark-3.x sourceset
used by the 3.4/3.5 profiles.

How are these changes tested?

  • python3 dev/ci/check-suites.py reports the six undeclared names (RED) before
    the removal, then prints All workflow-listed suites are declared and exits 0
    after it.

@github-actions github-actions Bot added build Build environment enhancement New feature or request area:ci CI/CD, GitHub Actions, build tooling labels Sep 22, 2026
dev/ci/check-suites.py only checked one direction: every suite in the source
tree must appear in a workflow. It never checked the reverse, so six suite
names that no longer exist stayed in the linux-test and macOS matrices.

ParquetReadV2Suite was removed in apache#3671, and none of the six has a class,
trait or object declaration in any sourceset. CI stays green only because the
suites are passed as -DwildcardSuites, which silently ignores unmatched names;
an explicit -Dsuites list fails with ClassNotFoundException.

Add the forward check and remove the six dead lines. ParquetReadV1Suite and
CometStringDecodeSuite are kept: the former is declared in ParquetReadSuite.scala
and the latter in the spark-3.x sourceset used by the 3.4/3.5 profiles.
@ErikBPF
ErikBPF force-pushed the ci/drop-nonexistent-suites branch from c120e89 to f4810f0 Compare September 22, 2026 01:16
@ErikBPF
ErikBPF marked this pull request as ready for review September 22, 2026 01:16

@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.

Summary

  • Prior state and problem: Both CI matrices listed six nonexistent suites that wildcardSuites silently ignored.
  • Design approach: Remove those entries and extend check-suites.py to check workflow names against Scala sources.
  • Correctness / compatibility analysis: All six removed prefixes match no declarations in the checkout. Remaining matrix names have matching package declarations. Checked ScalaTest 3.2.16 discovery behavior and relevant Spark sources across supported 3.4–4.2 profiles. No introduced P1/P2 issues found within this review.
  • Key design decisions: Scanning across source sets preserves version-specific suites and suites declared in differently named files. The implementation uses standard-library parsing without adding dependencies or unnecessary abstractions.
  • Implementation sketch: Add declaration collection and undeclared-name diagnostics, then remove six entries from each workflow.
  • Behavioral changes worth calling out: The checker now rejects the six stale entries. No active suite selection or Spark runtime behavior changes. The complete checker took approximately 0.36 seconds locally.
  • Suggested improvements: None at the requested P1/P2 bar.

Reviewed the full three-file diff from 9a4d5f28368d4fbb54ac23c14c5d52d5637ce26d to f4810f0d836aa26f4723f5af7d5ce40b69d823a4. The PR is non-draft. Routed skill: review-comet-pr; no sibling area skills apply to this CI-only change. Snapshot and live discussion checks contained no reviews, comments, or threads.

Exact-head CI: label passed. Comet CI and CodeQL report action_required, with no jobs executed. Hosted build/test validation is therefore unavailable.

Local validation: check-suites.py, check-ci-config.py, and git diff --check passed. Disposable fixtures restored the base workflows and produced exactly 12 expected diagnostics, verified rejection of an unknown suite, and passed after restoring the head workflows. No full Maven/native build or Spark test suite was run. The checkout remains unchanged.

dev/ci/check-suites.py only checked one direction: every suite in the source
tree must appear in a workflow. It never checked the reverse, so six suite
names that no longer exist stayed in the linux-test and macOS matrices.

ParquetReadV2Suite was removed in apache#3671, and none of the six has a class,
trait or object declaration in any sourceset. CI stays green only because the
suites are passed as -DwildcardSuites, which silently ignores unmatched names;
an explicit -Dsuites list fails with ClassNotFoundException.

Add the forward check and remove the six dead lines. ParquetReadV1Suite and
CometStringDecodeSuite are kept: the former is declared in ParquetReadSuite.scala
and the latter in the spark-3.x sourceset used by the 3.4/3.5 profiles.
Keep the verified refreshed tree while retaining the published head
as an ancestor, allowing a normal push without rewriting history.
@github-actions github-actions Bot added area:writer Native Parquet writer area:shuffle Shuffle (JVM and native) area:aggregation Hash aggregates, aggregate expressions area:scan Parquet scan / data reading area:expressions Expression evaluation area:ffi Arrow FFI / JNI boundary area:Iceberg area:udf area:memory Memory pools, reservations, OOM handling area:joins Join operators and dynamic filter pushdown labels Oct 2, 2026
@ErikBPF

ErikBPF commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor Author

Updated against pinned main 63fd1c9e. Rebase conflicts were resolved while retaining upstream matrix additions and CometUnboundedShuffleMemoryAllocatorSuite. Published ancestry is retained with an identical-tree merge, without force-pushing.

The reverse suite-declaration check remains: explicitly named matrix suites must resolve to real Scala suite declarations. Stale entries were removed from both Linux and macOS matrices; ParquetEncryptionITCase remains covered. There are 151 matrix entries per workflow, not 151 distinct Scala suites.

Verification on Apollo:

  • The original stale matrices produced 12 expected diagnostics; corrected matrices passed.
  • Deliberately invalid suite names produced the expected two diagnostics.
  • CI configuration, syntax, diff checks and workflow-only actionlint passed; independent source review found no introduced defect.

Full actionlint still reports unchanged ShellCheck findings in unedited shell fragments (SC2006, SC2086, SC2129); it is not being reported as fully green. No Scala/runtime change was made in this PR.

Updated head: 81a7b21b53d1be6a81244a8b8bb278d91f6e3c2a (tested tree unchanged from cb525947eeef4e3c13bacca9a0446a1607ef0bf4).

Current-main diff correction

The intended patch was rebased onto ca1c143a. An ordinary merge of actual main preserved public ancestry without force-pushing and produced exactly the clean rebased tree. GitHub Files Changed is now verified at 3 files, +35/-12, limited to the two workflow matrices and check-suites.py.

Suite/CI guards, bad-name and guard-knockout controls, syntax and workflow-only actionlint passed on this base. Upstream float-suite additions, allocator naming and encryption IT remain intact. There are now 152 matrix entries per workflow; the earlier 151 count belonged to the older base. Full ShellCheck still has unchanged upstream diagnostics, not a fully green verdict.

Updated head: ad24e03bb9d314b69c6d478ce204f98a71df3225.

@andygrove andygrove 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.

Thanks @ErikBPF

@andygrove
andygrove enabled auto-merge October 5, 2026 13:39
@andygrove
andygrove added this pull request to the merge queue Oct 5, 2026
Merged via the queue into apache:main with commit 3fa27c7 Oct 5, 2026
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:aggregation Hash aggregates, aggregate expressions area:ci CI/CD, GitHub Actions, build tooling area:expressions Expression evaluation area:ffi Arrow FFI / JNI boundary area:Iceberg area:joins Join operators and dynamic filter pushdown area:memory Memory pools, reservations, OOM handling area:scan Parquet scan / data reading area:shuffle Shuffle (JVM and native) area:udf area:writer Native Parquet writer build Build environment enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants