ci: label pull requests by changed paths and title prefix - #5762
ci: label pull requests by changed paths and title prefix#5762dwsmith1983 wants to merge 11 commits into
Conversation
Pull requests are labeled by hand today, and most merged ones carry no label at all. A labeler workflow now adds the area labels from the paths a pull request touches and the type label from its conventional commit prefix, the same scheme maintainers apply manually. Labels are only ever added, so hand edits stay, and the gating labels that trigger extra test runs are never touched. The path rules were checked against the forty open pull requests that carry area labels: 40 matches, 5 extra labels, 7 missed.
|
@andygrove I saw that you were hand labeling so added the github action for labeling prs. |
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
This replaces manual PR labeling with path-based area labels on open, push and reopen, plus conventional-title type labels on open. I reviewed both added files at 923b3640d301aed4872ffd6651025aab15464e7a against 7f1e00189b1ed86f1cb5acd872d97fce694482b1 (also the merge-base), including the current discussion. There is one P2 finding: a push can cancel the opening run before type labeling finishes, and the replacement run skips that step. A normally opened fix: PR can therefore remain without bug; the inline comment identifies the event interaction and a bounded fix.
The path configuration otherwise matches the stated approach. In the resolved labeler v7 source, all-globs-to-any-file requires one file to satisfy the positive and negative patterns together, so an unrelated file does not defeat the expression exclusions. The single brace patterns for area:ci and documentation allow different files to match different alternatives while requiring every changed file to match. The title table is a separate source of labels, so a docs: title can add documentation even when the path-only rule does not. With synchronization disabled, path labels accumulate across revisions; this intentionally preserves existing labels rather than making them a current-diff classification.
The fork boundary is appropriate for this workflow: there is no checkout or shell step, the labeler fetches configuration at context.sha, and pull_request_target supplies the trusted default-branch context. Titles are read as runtime data, not interpolated into script source, and the labels come from fixed mappings. All configured labels currently exist; pull-requests: write supports applying existing labels. This is a source-level assessment, not a fork/token integration test. (Event semantics, label API permissions.)
Static validation passed for both YAML files, duplicate-key checks and parsing the inline JavaScript. At the discussion cutoff, all four current-head Actions runs were action_required; subsequent focused job reads returned zero jobs, so there is no executed CI result to claim. I did not execute the workflow or actions, and did not independently rerun the author's historical 40-PR comparison. This changes only GitHub metadata automation: Spark 3.4/3.5/4.0/4.1 expression/operator semantics, modes, types, nulls, overflow, errors and fallback are outside the changed behavior. No maintained Spark/Delta source or Spark/JVM validation is claimed; the maintained 3.4/4.1 source gaps remain unqualified.
Performance
The added work is API access plus matching changed paths against a small fixed configuration and parsing a title. It adds no query execution, allocation or copying to Comet's runtime. Reusing the labeler avoids a second custom implementation of file pagination and matching; the title step sends no request when its mapped labels are already present in the event. No configured output is a CI gating label, and the existing CI preflight rejects non-gating label events if one is emitted. The lightweight runner is consistent with the existing issue-labeling workflow. There is no material engine-performance claim or hot-path change requiring a Spark microbenchmark; no runtime speedup is asserted.
Design
Keeping trusted path configuration separate from the small title mapping makes the behavior understandable, and using the target event supports fork PRs without checking out their code. Additive path labeling and open-only type labeling make the maintainer-edit policy explicit. The cancellation policy needs to respect that difference: path labeling can be replaced by a newer run, while the open-only title operation has no replacement on push or reopen. Preserve that opening operation, for example by separating its concurrency group from later events, without broadening title relabeling to overwrite later maintainer choices. Merely disabling active-run cancellation would still leave default pending-run replacement to consider.
Abstraction & complexity
A declarative configuration plus one short script is an appropriate level of abstraction. The per-file exclusions and all-files brace patterns earn their complexity by avoiding unwanted expression, CI and documentation labels, while overlapping subsystem labels such as Iceberg and scan remain intentional. There is no new engine abstraction, service, custom action or persistent state to maintain. Beyond protecting the one-time title operation from cancellation, I found no additional P1/P2 issue that warrants expanding this change.
| # maintainers label by hand. Only on open, so later hand edits are not overwritten, and | ||
| # not for dependabot, whose PRs already carry the dependencies label. | ||
| - name: Label by title prefix | ||
| if: github.event.action == 'opened' && github.actor != 'dependabot[bot]' |
There was a problem hiding this comment.
Correctness
[P2] Preserve the opened run until type labeling finishes
If a non-Dependabot PR is opened with a fix: title and no bug label, then receives a push while the opening run is still in its path-labeling step, the synchronize run has the same concurrency group and cancels it. This condition then skips title labeling in the replacement run; future pushes and reopens also skip it, so the missing type label is never recovered automatically. Keep the opening operation out of the cancellation group for later events, or otherwise ensure it completes once. Changing only cancel-in-progress to false is insufficient for an opening run that gets replaced while pending. This follows the documented concurrency behavior.
There was a problem hiding this comment.
Right, one group per pull request let a push cancel the opened run mid-way, and nothing later re-adds the type label. Fixed in 20aaf60 by keying the group on the event action as well, so synchronize runs replace only each other and the opened run always completes. Reopened runs get their own group for the same reason. Title relabeling itself stays open-only.
… group The title step runs on open only. With one concurrency group per pull request, a push that arrived while the opened run was still labeling paths cancelled it, and no later event adds the type label. Keying the group on the event action as well lets pushes replace only each other.
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
The cancellation defect from the previous review is fixed at 3d890dec964a99c023555342f3ce55be391c2bec: adding the event action to the group keeps synchronize and reopened runs from cancelling opened. I compared this update with 923b3640d301aed4872ffd6651025aab15464e7a and reviewed the full contribution against bb9e74020adc228e486f6f4d0fa68292b30bff31, which is also the current merge-base. The authored follow-up changes only the concurrency key and its explanation. The seven other incremental paths are inherited base changes.
One new P2 remains at the changed key: the event groups now permit concurrent label writers, and the path action replaces the complete label set. A push introducing a new area label can finish its last label read before the opening step adds bug, then overwrite that addition. The pinned labeler v7 source and shipped bundle both refresh labels before calling setLabels. That refresh narrows the window but does not make the read and replacement atomic. Thus sync-labels: false preserves labels it has observed, but cannot guarantee preservation of a concurrent addition. The new inline comment identifies this distinct failure after the cancellation fix.
Both YAML files pass static parsing with duplicate-key rejection, and the inline JavaScript passes syntax parsing. The unchanged configuration still uses API file pagination, trusted target-context configuration and a runtime title value; the follow-up adds no checkout, shell command, title interpolation into script source or token permission. Both action tags currently resolve to the same commits inspected previously, and every configured label exists. At the complete discussion cutoff, 2026-09-08T09:21:31.836Z, all three current-head Actions runs were action_required; focused reads returned zero jobs. No workflow/action execution, concurrent-event integration test or fork/token validation is claimed. The author's historical 40-PR comparison was not rerun.
This remains CI metadata automation. Spark 3.4/3.5/4.0/4.1 expression/operator semantics, types, nulls, overflow, error handling and fallback have no changed behavior in this PR contribution; no Spark/JVM or maintained-source runtime qualification is claimed. The maintained Spark 3.4/4.1 source gaps remain explicit.
Performance
Different event types may now run together, while repeated pushes still replace each other. This can duplicate a small amount of path matching and API work for the same PR, but adds no Comet query-runtime work or engine-performance claim. The relevant cost of the additional overlap is the lost label update above. Serializing these short metadata operations would be proportionate. No Spark benchmark is relevant to this change.
Design
Protecting the one-time opening operation is necessary, but the concurrency boundary also needs to protect the shared label set. Serialize the workflow's label mutations with a pending queue that preserves the opening run, or use a genuinely additive path-label operation. GitHub's current concurrency documentation describes queue: max with cancellation disabled as one bounded option (up to 100 pending runs). Simply turning cancellation off with the default single pending slot still permits replacement of a pending opening run. The open-only title policy can remain intact.
Abstraction & complexity
The additional group component is easy to read and the declarative path rules remain unchanged. The issue comes from the action's read/replace behavior, so a bounded adjustment to write ordering or update semantics is preferable to adding persistent state or a custom labeling framework. I found no other new P1/P2 in the authored follow-up.
| # the title step runs on open only, and a synchronize run that cancelled it would leave the | ||
| # type label missing with no later event to add it. | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number }}-${{ github.event.action }} |
There was a problem hiding this comment.
Correctness
[P2] Prevent overlapping runs from replacing the title label
Including the event action allows opened and synchronize to write labels concurrently. Despite sync-labels: false, labeler v7 refreshes the current labels and then replaces the complete set, using issues.setLabels. If a push introduces a new area label, its path run can finish that final read before the opening title step adds bug, then replace the labels after the addition and remove bug. The refresh preserves additions observed before the read, but cannot protect one arriving between the read and replacement. Later pushes/reopens still skip the title step, leaving it missing. Could we serialize label mutations while preserving the opening operation, or make the path update genuinely additive? Separating the event groups alone trades cancellation for this lost update.
There was a problem hiding this comment.
Agreed, separate groups traded the cancellation for a lost update. Now one group per pull request with queue: max and no cancellation (cancel-in-progress is rejected alongside it), so runs for the same pull request execute in order, every run completes, and the opened run's title step can no longer race a path run's read and replace. The cost is that rapid pushes queue a few seconds of labeling each instead of cancelling, bounded by the 100 pending runs GitHub allows per group.
There was a problem hiding this comment.
Preflight's actionlint (1.7.12) rejects queue, so the serialization now comes from a single group per pull request with cancellation, which keeps one writer at a time, plus a title step that can be repeated: it runs first on every event and adds the type label only while the pull request carries no type label at all. A run replaced or cancelled by a newer push is completed by that push, and a hand-picked replacement label stays. The one case that changes is a maintainer removing every type label on purpose; the next push adds one back from the title. Head df1b028, actionlint clean locally.
The labeler action reads the label set and writes it back whole, so runs for different events on the same pull request could overwrite each other's additions. Queuing every run in one group per pull request serializes the writes and still lets the opened run finish.
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
The concurrent-writer P2 is fixed at f078a9cb2e0014bfa77c63976fff1b647379c3f9. I compared the full contribution against bb9e74020adc228e486f6f4d0fa68292b30bff31, also the merge-base, and the follow-up against 3d890dec964a99c023555342f3ce55be391c2bec. Only the concurrency settings and their explanation changed. The path configuration, steps, triggers and permissions are unchanged.
All registered events now share one workflow-level group for the PR. That covers both the path action's read/replace phase and the opening title step, so a separate synchronize or reopened run cannot replace labels midway through the opening run. Removing active cancellation and using queue: max also avoids replacing an admitted pending opening run. The fix works whether the opening run or a synchronization run enters first. GitHub documents a maximum of 100 pending runs, with additional arrivals cancelled, and ordering by queue entry rather than event dispatch. This is a bounded coordination guarantee, not an unconditional promise that every run completes. (Concurrency semantics.)
The trigger list remains opened, synchronize and reopened. Title/body edits do not dispatch a run. Type labeling still uses the opening title once and skips Dependabot. Both action tags currently resolve to the previously inspected commits. The source still uses API file pagination and trusted target-context configuration, without checking out PR code or interpolating its title into script source. Coordination applies to this workflow's runs. Unrelated manual/API writers do not participate in its group.
Both YAML files pass static parsing and duplicate-key checks, and the inline JavaScript passes syntax parsing. At the 2026-09-08T12:24:57Z refresh, all three current-head Actions runs were action_required. Focused reads returned zero jobs, so no executed CI, workflow scheduling or fork/token integration result is claimed. The author's historical 40-PR comparison and per-run timing were not independently measured. No new or remaining P1/P2 was verified.
This follow-up changes only CI metadata automation. Spark 3.4/3.5/4.0/4.1 expression/operator behavior, modes, types, nulls, overflow, errors and fallback are outside the contribution. No Spark/JVM runtime qualification is claimed, and the maintained Spark 3.4/4.1 source gaps remain explicit.
Performance
Rapid pushes now retain labeling work in a queue, so each admitted event may repeat API reads and path matching instead of being cancelled. The 100-pending limit bounds the backlog, while separate PRs retain separate groups. This is a reasonable tradeoff for preserving the one-time title operation and serializing whole-set updates. There is no query-runtime work or engine speedup claim, and no relevant Spark microbenchmark requirement.
Design
The group now follows the shared resource being modified: one PR's label set. Applying it to the entire workflow closes the gap that sequential steps inside one run could not close across event runs. The queue setting addresses pending-run replacement at the same boundary. This resolves both reviewed failure mechanisms without changing the open-only title policy or adding a custom locking service.
Abstraction & complexity
One shared group and one queue setting are sufficient for the required coordination. The explanatory comment ties them directly to the action's replacement behavior and the one-time title step. No new action, persistent state or label-matching abstraction was added. I found no further update-specific simplification needed before merge.
…repeatable The Preflight actionlint does not know the queue setting. One group per pull request with cancellation keeps a single writer, and the title step now runs first on every event, adding the type label only while the pull request has none, so a run replaced or cancelled by a newer push loses nothing and a hand-picked type label stays.
641a78f to
df1b028
Compare
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Re-reviewed c33d22d9cc9b1f4945314eabf7cdaff5e3413215 against 6f25df70b3825b68a3f9eec01e67de36c0e0bcb8, also the merge-base, and compared the update with approved head f078a9cb2e0014bfa77c63976fff1b647379c3f9. The path rules are unchanged. The authored update restores cancellation, moves title labeling before path labeling and runs that step on every registered event unless the actor is Dependabot. The other 28 incremental files match upstream base changes.
The earlier open-only cancellation case is addressed because a later eligible run can now perform the title step. The shared workflow-level PR group also remains, addressing the different-event-group overlap. This replaces the prior bounded queue with cancellation and retry, so it does not promise completion of every event. The author's stated change to restore a type after all types are deliberately removed is explicit.
One new P2 is inline: the new existing-type guard reads event-time labels, so it can add a title-derived type despite a maintainer choice completed before the run starts. The current-state decision needs a fresh label read. This is separate from the prior concurrent-writer issue.
Both action tags currently resolve to the previously inspected commits. Triggers, permissions, trusted target-context configuration and the fixed label mapping are otherwise unchanged. There is no checkout of PR code or interpolation of its title into script source. The title action now precedes the path action's API refresh, so that refresh cannot supply current labels to the new guard.
Both YAML files pass parsing with duplicate-key rejection, and the inline JavaScript passes syntax parsing. Three exact-inline-script checks with an in-memory API double confirmed normal addition, skipping a type captured in the event, and the stale-event wrong addition. These are component checks under local Node 22, not action Node 24 or live workflow execution. At 2026-09-08T16:10:35.949114+00:00, all three current-head workflow runs were action_required, with zero jobs. The synthetic merge has the expected base/head parents and the same full tree as this head. The author's actionlint and historical 40-PR results were not independently rerun.
This update changes GitHub metadata automation only. Spark 3.4/3.5/4.0/4.1 expression/operator semantics, modes, types, nulls, overflow, errors and fallback are outside the contribution. No Spark/JVM qualification is claimed, and the maintained Spark 3.4/4.1 source gaps remain explicit.
Performance
Cancellation reduces repeated queued path matching and API work compared with the previous queue: max approach. The new title check is small. A fresh PR-label read adds bounded metadata work needed to implement the current-state policy. There is no query-runtime change or engine speedup claim requiring a Spark microbenchmark.
Design
Keeping title addition before path labeling and using one PR group makes the retry sequence understandable. The remaining correction belongs at the state check: a repeatable operation must consult current labels when deciding whether it is still needed. The documented default pending-slot replacement and cancellation behavior do not refresh event payloads. (Concurrency semantics.)
Abstraction & complexity
The type-label set is a small, appropriate addition to the existing script. A current-label read and a focused regression fit that same step. No custom locking service, persistent state or new labeling framework is needed for this finding.
| const present = new Set(context.payload.pull_request.labels.map((l) => l.name)); | ||
| if ([...present].some((l) => typeLabels.has(l))) { | ||
| core.info('A type label is already present, leaving it as is'); | ||
| return; |
There was a problem hiding this comment.
Correctness
[P2] Read current labels before deciding no type label exists
Could this guard fetch the PR's current labels rather than using context.payload.pull_request.labels? That is the triggering webhook payload, so a synchronize event can contain no type labels even when a maintainer has finished adding enhancement before its queued run starts. For a fix: title, this check still passes and adds bug alongside that choice. Label changes do not dispatch this workflow, and the later path step preserves both labels. This does not require overlapping runs or a manual write during the check, and differs from the documented case of intentionally removing every type label. Reading current labels before the decision would respect an already-completed maintainer choice. Please add a regression where event labels are empty but current labels contain a type. The exact inline script confirmed this branch with an in-memory API double, with no real label writes.
There was a problem hiding this comment.
Yes, the payload is stale by the time a queued run starts. In d7e1051 the decision reads the pull request's current labels through the API before deciding, and the logic now lives in dev/ci/pr-type-label.mjs so it can be tested: dev/ci/pr-type-label.test.mjs covers the case you describe (event labels empty, current labels already carry a type, nothing added) alongside the normal addition, a title without a prefix, and an already-present label, and Preflight runs it. The workflow checks out the base branch to read that module; under pull_request_target the default ref is the base commit, and nothing from the pull request is read or executed.
The title step judged whether a type label was present from the event payload, so a label a maintainer added before a queued run started was not seen and a second type label followed. The decision now lives in dev/ci/pr-type-label.mjs, reads the pull request's current labels through the API, and has a node test that Preflight runs, including the case where the event carried no labels but the pull request already has a type.
There was a problem hiding this comment.
Correctness
Re-reviewed 6825b7caeb29a10870fbbb38f360468b2883bdb7 against authoritative base 8e6846850c525506dd2b9194f2014e8acd2ab60a, including the update from c33d22d. The five-file contribution now adds a current-label API read, a shared policy helper and four tests wired into Preflight. The path configuration is unchanged. The other 17 incremental paths match the corresponding base changes.
The prior P2 is addressed. The workflow passes freshly fetched label names to the helper, so a maintainer's existing type suppresses a title-derived addition even when the event carried no type. An in-memory control using the previous inline script confirmed the old wrong addition. The exact current inline script preserves that maintainer choice. A failed read stops before addition. Repeat runs consult current state, while deliberately removing all types permits a later eligible run to restore the title-derived type.
The new checkout uses the trusted repository's default event ref, with persisted credentials disabled. The helper is not imported from a supplied PR-head ref. GitHub documents pull_request_target as running in the default-branch context. The existing permissions and title-before-path ordering remain unchanged. (Event semantics.)
All four repository tests, 15 exact-inline workflow fixtures and 99 policy assertions passed under local Node 22.22.2. Three YAML files also passed parsing with duplicate-key rejection. These fixtures use in-memory API methods: they do not execute Actions, Node 24 or cancellation scheduling. At the 2026-09-08T17:39:47Z CI refresh, all three current-head workflows were action_required, with zero head or merge checks. The synthetic merge has the assigned parents and the same tree as HEAD. That establishes source identity, not CI success.
No new or remaining verified P1/P2 was found. The three historical threads remain unresolved and outdated in GitHub despite the source fixes. This metadata-only change does not alter Spark expression or operator behavior, and no Spark runtime qualification is claimed.
Performance
The fresh label read adds one metadata request per eligible title step. A single page of up to 100 labels covers the repository's current 54-label inventory. The new default shallow checkout adds setup work to load the helper and trusted configuration. Neither cost changes query execution, and there is no engine performance claim requiring a Spark benchmark.
Design
Fetching state immediately before applying the policy directly fixes the reviewed stale-event case. The existing shared PR concurrency group and title-before-path sequence keep retries understandable. Recovery still depends on a later eligible run completing. Cancellation does not guarantee every event completes, and the read/add sequence is not atomic against unrelated manual writes after the read. Those boundaries do not invalidate the fixed case, where the maintainer's change finished before the fresh read.
Abstraction & complexity
The small pure helper gives the workflow and Preflight tests one implementation of prefix mapping and existing-type preservation. API access remains in the workflow, and tests can exercise policy without repository writes or service mocks. This extraction is proportionate to the regression and introduces no new dependency or persistent coordination mechanism.
3 participants
Which issue does this PR close?
Closes #5761.
Rationale for this change
Pull requests are labeled by hand. Open pull requests mostly carry a type label from the title prefix plus one or two
area:*labels, while most merged pull requests carry none, so the labels are not a reliable view of what changed where and each one costs a maintainer a click. Issues already getrequires-triageautomatically; this does the equivalent for pull requests using the scheme maintainers apply manually.What changes are included in this PR?
.github/labeler.yml: path rules for thearea:*labels (shuffle, scan, writer, aggregation, expressions, joins, ffi, memory, udf, Iceberg, ci) anddocumentation.area:cianddocumentationapply only when every changed file matches, since most pull requests touch a workflow file or a doc in passing. The expressions rule excludes the aggregate and UDF paths so those get their own label..github/workflows/label_prs.yml: runsactions/labeleronpull_request_targetso fork pull requests are labeled too. Nothing from the pull request is checked out or executed; the changed file list comes from the API.sync-labelsis off, so labels are only ever added and hand edits stay. A second step adds the type label from the conventional commit prefix (feat,refactor,choretoenhancement;fixtobug;perftoenhancementandperformance;testtoenhancementandtest;docstodocumentation;ciandbuildtoenhancementandbuild) on open only, and skips dependabot.The gating labels (
run-spark-*-tests,run-iceberg-tests,skip-ci) are not in either file, so the workflow cannot apply them.ci.ymlalready ignoreslabeledevents for anything but those labels, so the added labels produce skipped preflight runs and nothing else.How are these changes tested?
The path rules were run against the 40 open pull requests that carry
area:*labels today: 40 label matches, 5 extra labels, 7 missed. The misses are judgment calls such as a change confined to the planner being labeledarea:expressions, which the rules cannot see; the extras are a second area on a pull request that touches two subsystems. Both YAML files parse, and the action's glob semantics (any-glob-to-any-file,all-globs-to-any-filewith negations,any-glob-to-all-fileswith a brace glob) were checked against the labeler's source. The workflow itself only runs once merged, since it triggers onpull_request_target.