Repository navigation
ci: label Delta pull requests with area:Delta - #6965
Conversation
The rule lists the Delta paths explicitly, since a *[Dd]elta* glob would also match the Iceberg delta-write code.
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the complete diff from 8a4029ccf9a079385224bef8aca95d561993b221 to 571f9370366d0de29e64c488dfa8628de0274566. The PR is open and not a draft. Read AGENTS.md, the discussion snapshot, live reviews/comments/threads, and issue #6964. Routed skill: .ai/skills/review-comet-pr/SKILL.md. No sibling review skill applies to this configuration-only change.
Summary
- Prior state and problem: Delta changes lacked a dedicated area label. Contrib files received
area:scan, while native Delta files lacked a Delta-specific classification. - Design approach: Adds
area:Deltathrough explicit path rules, avoiding a broad name glob that would also classify Iceberg delta-write code. - Correctness: The bundled
actions/labeler@v7parser and matcher accepted the configuration. Across all 2,689 tracked files, the rule matched 33 Delta files and excluded all 12 Iceberg delta-write paths. Mixed-file and boundary cases passed. No Spark expressions or operators changed, so Spark-source semantic comparison is not applicable. - Compatibility analysis: Existing rules are structurally unchanged against both the requested base and latest release branch
branch-1.1. The intended difference is the additional Delta label. Spark results, supported versions, and native fallback behavior are unaffected. - Key design decisions: Explicit Delta paths distinguish Delta Lake from Iceberg terminology. The existing
sync-labels: falsepolicy continues preserving labels. - Implementation sketch: Eight patterns cover both contrib modules, three native files, Delta workflows, the gate script, and the Delta guide.
- Performance: The addition extends the existing matcher by eight patterns. It adds no workflow job or Spark runtime work. No material performance concern was identified.
- Design: Reusing the existing declarative labeler is straightforward and appropriately scoped. No separate script or broader abstraction is needed.
- Abstraction & complexity: The explicit list is easy to inspect and avoids complicated exclusions. No unnecessary abstraction or P1/P2 complexity issue was identified.
- Behavioral changes worth calling out: Matching PRs gain
area:Deltaalongside existing labels after this configuration reaches the base branch. The already-disclosed label-creation prerequisite remains unresolved. - Suggested improvements: Complete the documented prerequisite by creating
area:Deltabefore merge. No additional code change meeting the P1/P2 reporting bar was identified.
Existing blocker: The repository label endpoint returned HTTP 404, and the final label-list check still contained no area:Delta. .github/workflows/label_prs.yml grants pull-requests: write, but not issues: write. The labeler documentation and implementation confirm that creating a missing label requires the latter permission. A matching Delta PR would therefore fail the combined path-label update, preventing its newly calculated path labels from being applied. This concern is already disclosed in the PR body, so it is not repeated as a new inline finding.
Exact-head CI: Label pull requests succeeded. Comet CI, CodeQL, and Check PR Title report action_required, so they provide no passing validation verdict. The successful label workflow reads the base configuration and does not exercise this addition.
Validation limits: Local validation used the actual bundled v7 parser/matcher under Node 18, rather than the action's Node 24 runtime. No live label mutation or JVM/native build was performed. The working tree remains unchanged.
No introduced P1/P2 issues found within this review beyond the already-disclosed label-creation prerequisite.
|
I created the label |
Which issue does this PR close?
Closes #6964.
Rationale for this change
Delta pull requests get no area label of their own. A change under
contrib/**is labeledarea:scan, and Delta code outsidecontrib/, such asnative/core/src/execution/delta_dv.rs, gets nothing Delta specific, while Iceberg pull requests getarea:Iceberg.What changes are included in this PR?
An
area:Deltarule in.github/labeler.yml. It lists the Delta paths explicitly (both contrib modules, the three native Delta files undernative/core/src/execution/, the Delta workflows, the gate script and the Delta user guide page) instead of copying the Iceberg rule's name glob, since*[Dd]elta*would also match the Iceberg delta-write code inspark/src/main/scala/org/apache/comet/iceberg/and its shims.How are these changes tested?
I ran each glob through minimatch with
dot: true, as the labeler does, against the file list onmain. Every file whose path contains "delta" matches except the Iceberg delta-write files. Run over the changed files of recent pull requests, the rule labels #6945, #6932, #6838 and #6270, and leaves #5869 (delta encoding for cached longs) and #6664 (Iceberg writes) alone.The
area:Deltalabel does not exist yet, andlabel_prs.ymlgrants onlypull-requests: write, so the labeler cannot create it. Its singlesetLabelscall would then fail and drop the other path labels too. Please create the label before merging, for example with the color and style of the other area labels:gh label create area:Delta --repo apache/datafusion-comet --color C5DEF5 --description "Delta Lake contrib (native scan, deletion vectors)"