ci: move the Spark 4.1 sql_hive shards behind the merge queue - #5871
ci: move the Spark 4.1 sql_hive shards behind the merge queue#5871andygrove wants to merge 1 commit into
Conversation
Keep catalyst and the three sql_core shards in the PR tier and run the three sql_hive shards only in the merge queue, or on a pull request that carries the new run-spark-4.1-hive-tests label. Over Aug 12 to Sep 11 the hive shards cost about 65 runner-hours a day on pull requests and were the only failing job on 7 PR runs, against 33 for the sql_core shards, and their 67-minute shard set the PR tier's wall clock at about 110 minutes. The reusable workflow gains a `modules` input (all, core or hive). The matrix rows move from a workflow literal to dev/ci/spark-sql-modules.py, which the build job runs to publish the selected rows as a job output, because a job-level `if:` cannot see the matrix context. ci.yml folds two POLICY outputs, spark_4_1 and the new queue-only spark_4_1_hive, into that one input so the queue still gets every module from a single build. Part of apache#5870.
sunchao
left a comment
There was a problem hiding this comment.
Correctness
At 3fc81e05 against c369d04b, this implements step 2 of #5870: ordinary pull requests run Catalyst and the three sql_core shards, while the merge queue retains all seven Spark 4.1 shards. A PR carrying run-spark-4.1-hive-tests runs all seven on its next commit event. Adding that label starts a Hive-only run.
The two routing outputs share the same path-filter list, and the caller selects core, hive or all from their combination. I checked the label-only, unrelated-label, merge-group, push and manual-dispatch boundaries. The moved rows preserve the original test commands, tag filters and memory limits. Spark 3.4, 3.5 and 4.0 callers omit the new input and retain its all default. This changes scheduling, not Spark expression/operator behavior or the SQL test diffs. The existing Required Checks dependency still covers the single Spark 4.1 caller, and label runs retain their separate aggregate check name.
Validation
The repository CI-config check passed locally. A focused probe passed 100 event/path combinations, checked the old matrix against all seven generated rows, and exercised default/invalid CLI input, manual dispatch and output-file append behavior. The probe executes the actual Python routing and selector code, with a bounded local evaluation of the extracted workflow expressions. It does not replace hosted event execution.
Current CI expanded exactly four core shards and passed them. Preflight ran the config check and actionlint. The inspected sql_core-1 log reports 12,867 tests passed, 64 canceled and 812 ignored. Its native and JVM artifact digests match the producer. CI checked out f8742d65, with parents c369d04b and this head, and a full tree equal to the head. The later generated merge da0bbe0d preserves all seven changed files and .asf.yaml, but was not the checkout in that run. No hosted Hive-only or merge-group run was triggered for this review. I found no P1/P2 issue.
Performance
Removing three Hive execution rows from ordinary PR runs reduces repeated test work without adding another build job to the queue. The queue's core and Hive shards still share one native/JVM producer. The build continues compiling Hive test classes and packing the complete Spark artifact, so the saving comes from omitted test runners rather than eliminating all Hive build cost.
The roughly 65 runner-hours/day and 10-minute feedback improvement are estimates reported in the PR and issue from historical runs. I verified the four-versus-seven row selection, not those aggregate savings or a matched before/after timing. There is no query-runtime change that needs an expression microbenchmark.
Design
Keeping one reusable-workflow call avoids duplicating the expensive build and its artifacts. Selecting the rows before matrix expansion fits GitHub's job-output matrix pattern. The all default preserves existing callers, and rejecting an unknown selection before toolchain/build work gives configuration mistakes a direct failure.
The intended tradeoff is later detection of Hive-specific regressions on unlabelled PRs. Queue coverage remains present, and the documented label provides early coverage for Hive changes. The new policy cases and partition check make both the event split and shard inventory explicit.
Abstraction & complexity
The small Python module earns its place by holding the seven existing rows and producing the selected JSON matrix. Normalizing row keys keeps uncapped rows compatible with the existing heap/metaspace lookups. The second policy output is a routing decision for the same job, so it does not create another lifecycle or artifact producer.
The shared filter-list reference prevents core/Hive path rules from drifting, while the caller's short selection expression has only three reachable modes. The change stays within the existing CI structure. I found no additional abstraction or simplification that warrants a pre-merge finding.
|
Thanks for the review @sunchao |
Which issue does this PR close?
Part of #5870 (step 2 of the plan there). It does not close the issue.
Rationale for this change
#5843 left the whole
Spark SQL Tests (Spark 4.1)workflow in the PR tier. Over Aug 12 to Sep 11 (1,288 executed CI runs on pull requests, 168 push-to-main runs as the flake baseline):catalyst+sql_core-1/2/3(+ build)sql_hive-1/2/3The hive shards are the worst value in the PR tier by runner-hours per unique catch, and
sql_hive-2at 67 minutes after the 41 minute build is what sets the PR tier's wall clock. Moving them to the queue saves about 65 runner-hours a day and brings PR feedback down to about 100 minutes, while the 33 catches a month fromsql_corestay on pull requests.What changes are included in this PR?
spark_sql_test_reusable.ymlgains amodulesinput (all,coreorhive, defaultall). The matrix rows move from a workflow literal todev/ci/spark-sql-modules.py; thebuildjob runs it and publishes the selected rows as a job output thatspark-sql-testreads withfromJSON, the same pattern the Iceberg reusable workflow uses for its shards. A job-levelif:cannot readmatrix, which is why the rows are left out rather than skipped.dev/ci/compute-changes.pyadds aspark_4_1_hiveoutput with the same path filters asspark_4_1and POLICY["queue", "label:run-spark-4.1-hive-tests"].spark_4_1stays["pr", "queue"].ci.ymlfolds the two outputs into the onespark_4_1call, so the queue still gets every module from a single build: a plain PR run getscore, a merge group run (or a PR carrying the label) getsall, and alabeledrun for the new label getshiveonly.dev/ci/check-ci-config.pygains the two policy cases for the label and a check thatcoreandhivepartitionall..github/workflows/README.mdanddocs/source/contributor-guide/ci.mddocument the new tier split and label.The
run-spark-4.1-hive-testslabel has been created in the repository.Other Spark versions are unaffected:
spark_3_4,spark_3_5andspark_4_0do not passmodulesand keep running all seven rows in the queue.How are these changes tested?
python3 dev/ci/check-ci-config.pypasses, including the new label cases and the module partition check.actionlintpasses. It is what caught the first attempt, a job-levelif:onmatrix.module.name, which is not a valid context there.dev/ci/spark-sql-modules.py --modules core|hive|allprints the expected rows and rejects anything else.compute()plus theci.ymlexpression: PRsynchronizeselectscore, PR with the label selectsall,labeledwith the new label selectshive,merge_groupselectsall,pushdoes not call the workflow.Spark SQL Tests (Spark 4.1).