You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "sarutak (via GitHub)" <gi...@apache.org> on 2026/10/22 14:00:10 UTC
[PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
sarutak opened a new pull request, #59708:
URL: https://github.com/apache/spark/pull/59708
### What changes were proposed in this pull request?
`dev/merge_spark_pr.py` extracts the JIRA ID(s) associated with a PR from its title. Previously it scanned the entire title with `re.findall("SPARK-[0-9]{4,5}", title)` in three places:
- `resolve_jira_issues` (resolves the ticket on merge),
- the revert path in `update_jira_for_pr` (reopens/reverts the ticket), and
- the Epic/Umbrella blocker and JIRA-match check in `main()`.
Because it matched anywhere in the title, a `SPARK-NNNNN` that appears in the title *body* — e.g. a reference to a related ticket — was mistaken for the ticket the PR resolves.
This PR adds `Title.jira_ids()` and a small `leading_jira_ids()` helper that return only the `SPARK-NNNNN` ids appearing as *leading* bracket tags, reusing the existing `Title.parse()` that already distinguishes leading tags from body text, and switches the three call sites to use it.
### Why are the changes needed?
A PR title may legitimately mention another ticket in its body. For example [#59587](https://github.com/apache/spark/pull/59587):
```
[SPARK-60328][SQL][TESTS] Use a unique jar copy in the HiveDDLSuite SPARK-34261 test
```
The old code extracted `['SPARK-60328', 'SPARK-34261']`, so merging the PR would also try to resolve the unrelated `SPARK-34261` and would prompt to verify it as a linked ticket. Only the leading tag `SPARK-60328` is the ticket this PR resolves.
### Does this PR introduce _any_ user-facing change?
No. This only affects the committer-side merge script; it is not part of any released artifact.
### How was this patch tested?
- Added doctests for `Title.jira_ids()` and `leading_jira_ids()`, including the body-reference case. All existing doctests continue to pass (`python3 -m doctest dev/merge_spark_pr.py`).
- Verified against the title of #59587: the helper returns `['SPARK-60328']` whereas the old `re.findall` returned `['SPARK-60328', 'SPARK-34261']`.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Kiro CLI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6343253163
Thanks for the thorough review, @dongjoon-hyun. Items 1 to 3 were real regressions and your analysis was spot on. Pushed 2233a6b.
**1, 2, 4**: Added `_unwrap_title()` that strips both `Revert "..."` and `Reapply "..."` in one place, and route all three call sites through it (via `leading_jira_ids`). So the resolve path gets a default id for Reapply PRs again, and the Epic/Umbrella blocker and the match prompt run for Revert/Reapply PRs again. This also fixes the pre-existing gap you noted where the skip-tag check did not see `[MINOR]` in `Reapply "[MINOR]..."`.
**3, 6**: On a `ValueError`, `leading_jira_ids` now falls back to `_leading_spark_brackets()`, which collects the run of leading `[SPARK-N]` brackets (stopping at the first non-SPARK bracket, so body ids are still ignored). So `[SPARK-48455][DOCS][MINOR] ...` keeps its id. Docstring updated to describe both the wrapper handling and the fallback.
**5**: Added doctests for `Revert "..."` / `Reapply "..."` and for `[SPARK-1234][SQL][MINOR] Fix` on `leading_jira_ids`/`_unwrap_title`/`_leading_spark_brackets`, and changed the `update_jira_for_revert` doctest's body id to a distinct `SPARK-9999` so it actually exercises body-id exclusion.
**8**: `Title.jira_ids` now uses `list(dict.fromkeys(...))`.
**9**: Dropped the overlapping examples and the `re.findall` comparison from `Title.jira_ids`; kept the examples on `leading_jira_ids`.
**7**: One note. I kept `jira_ids = leading_jira_ids(title)` rather than reusing `parsed.jira_ids()`, because `leading_jira_ids` adds the unwrap and parse-failure fallback that `parsed` alone does not have. It does mean the unwrapped title is parsed twice in `update_jira_for_pr` (once for ids, once for tags). Happy to refactor into a single parse-plus-fallback helper returning both if you would prefer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "sarutak (via GitHub) via reviews" <re...@spark.apache.org>.
sarutak commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6535352850
Thank you, @dongjoon-hyun !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59708:
URL: https://github.com/apache/spark/pull/59708#discussion_r4349342076
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2181,30 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, this only returns
+ ids that appear as leading bracket tags, so a SPARK-NNNNN reference in the
+ title body is not mistaken for the ticket the PR resolves. Titles that do
+ not start with a recognized leading tag (and so cannot be parsed) yield an
+ empty list.
+
+ >>> leading_jira_ids("[SPARK-1234][SQL] Fix something")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[SPARK-1234][SQL] Follow up to SPARK-5678")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[MINOR][SQL] Fix typo")
+ []
+ >>> leading_jira_ids("No leading tag, mentions SPARK-5678")
Review Comment:
Minor: none of the new doctests use a wrapped title, which is where this change behaves differently. The `update_jira_for_revert` doctest (L2905) says reapplies "still resolve the issue", but it mocks `resolve_jira_issues`, so all 227 doctests pass while `leading_jira_ids('Reapply "[SPARK-1234][SQL] Fix"')` returns `[]`. Could you add doctests for `Revert "..."` and `Reapply "..."` titles here, and one for a title such as `[SPARK-1234][SQL][MINOR] Fix` that starts with an id but fails a later check?
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2181,30 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, this only returns
+ ids that appear as leading bracket tags, so a SPARK-NNNNN reference in the
+ title body is not mistaken for the ticket the PR resolves. Titles that do
+ not start with a recognized leading tag (and so cannot be parsed) yield an
Review Comment:
Nit: this says only titles that do not start with a recognized leading tag yield `[]`. It is also `[]` for titles that start with `[SPARK-N]` but fail a later check (e.g. `[SPARK-1234][MINOR] Fix`), and for every `Revert "..."` / `Reapply "..."` title. Could you make the docstring match the behavior, or the behavior match the docstring as suggested on L2203?
##########
dev/merge_spark_pr.py:
##########
@@ -1868,7 +1868,7 @@ def grant_contributor(self, user):
def resolve_jira_issues(title, merge_branches, comment, title_components=()):
- jira_ids = re.findall("SPARK-[0-9]{4,5}", title)
+ jira_ids = leading_jira_ids(title)
Review Comment:
**Reapply PRs no longer get a default JIRA id here.** `main()` keeps Revert/Reapply titles verbatim (L2432), so for a Reapply PR `title` is `Reapply "[SPARK-1234][SQL] Fix"`. `Title.parse` rejects it because it does not start with a bracket tag, so `leading_jira_ids` returns `[]` and the `len(jira_ids) == 0` branch runs. Before this PR, `re.findall` found `SPARK-1234` and passed it as the default. Now the committer sees `Enter a JIRA id []:`, and pressing Enter prints `JIRA ID not found, skipping.`, so the reapplied ticket is not resolved and gets no fix version. I confirmed this by calling `update_jira_for_pr` on the base commit and on 85aa4ba with `resolve_jira_issue` mocked. The `update_jira_for_revert` doctest (L2905, "reapplies still resolve the issue") does not catch it because it mocks `resolve_jira_issues` as a whole. Could you strip the `Reapply "..."` wrapper before extracting the ids?
##########
dev/merge_spark_pr.py:
##########
@@ -2546,7 +2595,7 @@ def main():
continue_maybe(msg)
# asf_jira is guaranteed to be set here: initialize_jira() fails fast otherwise.
- jira_ids = re.findall("SPARK-[0-9]{4,5}", title)
+ jira_ids = leading_jira_ids(title)
Review Comment:
**Revert and Reapply PRs now skip both JIRA checks below.** For those PRs `title` is still the verbatim `Revert "..."` / `Reapply "..."` string, because the normalization at L2432 is skipped. So `leading_jira_ids(title)` is always `[]`, and the Epic/Umbrella blocker and the "Does SPARK-N match this PR?" prompt never run for them. Before this PR, `re.findall` found the wrapped id, so both checks ran. This matters most for a Reapply PR, which resolves the ticket again. For a Revert PR, the match prompt was the last check before `revert_jira_issue` removes fix versions and offers to reopen the ticket. The PR description says this call site was switched, but not that these PRs now skip the checks. Could you extract the ids from the unwrapped title here as well?
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2181,30 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, this only returns
+ ids that appear as leading bracket tags, so a SPARK-NNNNN reference in the
+ title body is not mistaken for the ticket the PR resolves. Titles that do
+ not start with a recognized leading tag (and so cannot be parsed) yield an
+ empty list.
+
+ >>> leading_jira_ids("[SPARK-1234][SQL] Fix something")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[SPARK-1234][SQL] Follow up to SPARK-5678")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[MINOR][SQL] Fix typo")
+ []
+ >>> leading_jira_ids("No leading tag, mentions SPARK-5678")
+ []
+ """
+ try:
+ return Title.parse(title).jira_ids()
Review Comment:
**Any later parse error now drops a clearly leading id.** `Title.parse` also raises for titles that start with `[SPARK-N]` but break a later rule, e.g. `[SPARK-48455][DOCS][MINOR] ...` or `[SPARK-50566][MINOR][SS] ...`. Returning `[]` here loses ids the old regex found. In master's history, 157 of the 39,633 subjects that start with `[SPARK-` fail `Title.parse`. A Revert PR for any of them now calls `revert_jira_issue(branches, "")` instead of passing the id, so the committer must retype it or the fix-version removal is skipped. Could you fall back to collecting the leading `[SPARK-N]` brackets (or the old regex) when parsing fails, instead of returning `[]`?
##########
dev/merge_spark_pr.py:
##########
@@ -1908,7 +1908,7 @@ def update_jira_for_pr(pr_num, title, merge_branches, title_components, is_rever
print()
continue_maybe("Would you like to update an associated JIRA?")
if is_revert:
- jira_ids = list(dict.fromkeys(re.findall("SPARK-[0-9]{4,5}", jira_title)))
+ jira_ids = leading_jira_ids(jira_title)
Review Comment:
Minor: the root cause of the comments on L1871 and L2598 is that only this function unwraps a title, and only for `Revert "` (L1891, `title[len('Revert "') : -1]`). `Reapply "..."` is never unwrapped, and `main()` and `resolve_jira_issues` get the wrapped title. The same gap, already there before this PR, means the skip-tag check at L1892-L1898 does not see `[MINOR]` in `Reapply "[MINOR][SQL] ..."`. Could you unwrap `Revert "..."` / `Reapply "..."` in one place, e.g. inside `leading_jira_ids`, or as a `jira_title` computed once in `main()` and passed to all three call sites?
##########
dev/merge_spark_pr.py:
##########
@@ -1908,7 +1908,7 @@ def update_jira_for_pr(pr_num, title, merge_branches, title_components, is_rever
print()
continue_maybe("Would you like to update an associated JIRA?")
if is_revert:
- jira_ids = list(dict.fromkeys(re.findall("SPARK-[0-9]{4,5}", jira_title)))
+ jira_ids = leading_jira_ids(jira_title)
Review Comment:
Nit: `jira_title` is already parsed into `parsed` at L1893, so this parses it a second time, with its own error handling. Could you set the ids from `parsed.jira_ids()` inside that `try` block (and `[]` in the `except`) and reuse them here?
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2181,30 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, this only returns
Review Comment:
Nit: three of the four `Title.jira_ids` doctests (L2159-L2166) repeat the `leading_jira_ids` doctests on the same inputs, and this sentence explains the helper by the `re.findall` call this PR deletes, which will mean little once it is gone. Could you keep one set of examples and drop the comparison, since the PR description already explains it?
##########
dev/merge_spark_pr.py:
##########
@@ -2148,6 +2148,31 @@ def parse(cls, raw: str) -> "Title":
raise ValueError("[SPARK-NNNNN], [MINOR], and [TRIVIAL] cannot coexist")
return cls(leading, components, text)
+ def jira_ids(self) -> List[str]:
+ """The leading SPARK-NNNNN JIRA ids, in order, without duplicates.
+
+ Only the ids that appear as leading bracket tags count; SPARK-NNNNN
+ mentions in the title body (e.g. a reference to a related ticket) are
+ deliberately excluded, since the body is not the ticket this PR
+ resolves. [MINOR]/[TRIVIAL] markers in ``leading`` are dropped.
+
+ >>> Title.parse("[SPARK-1234][SQL] Fix something").jira_ids()
+ ['SPARK-1234']
+ >>> Title.parse("[SPARK-1234][SPARK-5678][SQL] Fix").jira_ids()
+ ['SPARK-1234', 'SPARK-5678']
+ >>> Title.parse("[SPARK-1234][SQL] Follow up to SPARK-5678").jira_ids()
+ ['SPARK-1234']
+ >>> Title.parse("[MINOR][SQL] Fix typo").jira_ids()
+ []
+ """
+ seen = set()
Review Comment:
Nit: the order-preserving dedup could be `return list(dict.fromkeys(t for t in self.leading if _SPARK_ID_RE.match(t)))`, the idiom this PR removes from the revert path and that `main()` uses for components (L2452). Could you use it here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6479774885
@dongjoon-hyun Gentle ping.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6525275854
Thank you, @sarutak .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6370124477
Any other feedback, @dongjoon-hyun ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6371769688
Pushed 3b0e10e with two small changes of my own, and updated the PR description. No behavior change for real titles.
- The nested-wrapper cases in the `update_jira_for_revert` doctest now use `Revert "Reapply "...""` instead of `Revert "Revert "...""` with `is_revert=True`. `Revert "Reapply "...""` is a revert both by its outermost wrapper, which is how `main()` decides today, and by wrapper nesting, so the doctest no longer pins the revert path for a re-land, which the planned wrapper-parity change will fix.
- `_BARE_SPARK_ID_RE` now adds `re.ASCII`, so its case-insensitive match no longer accepts Unicode lookalikes such as U+017F for `s` or U+212A for `K`. That makes "the same ASCII digits and case as the bracket path" in my previous comment exactly true. I also added a doctest for a 6-digit id.
The description now also notes that an id may have any number of digits (the old regex missed ids below `SPARK-1000` and read `SPARK-123456` as `SPARK-12345`), and that the only wrapped subjects in master's history that now yield fewer ids drop a body reference. It also corrects a mistake that was in both the description and my previous comment: `Revert "[WIP] [SPARK-3996]: Shade Jetty in Spark deliverables"` is from 2015, not 2014.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6525244372
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/6664c01e9b924f7673448ed2340099bb1795e411
*Posted by `merge_spark_pr.py`*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6499485474
Could you resolve the conflicts, @sarutak ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "spark-test (via GitHub)" <gi...@apache.org>.
spark-test commented on code in PR #59708:
URL: https://github.com/apache/spark/pull/59708#discussion_r4372824565
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,92 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip a ``Revert "..."`` / ``Reapply "..."`` wrapper from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping here
+ lets the id-extraction and skip-tag logic see them. A title without such a
+ wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ """
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
Review Comment:
Thanks, confirmed at 3b0e10e: `_unwrap_title` now strips every `Revert "..."`/`Reapply "..."` layer, so `leading_jira_ids('Revert "Revert "[SPARK-1234][SQL] Fix""')` returns `['SPARK-1234']` again. The `_unwrap_title` doctests cover the nested and mixed cases, and the `update_jira_for_revert` doctest checks that a nested id reaches the revert path. Agreed that deciding revert vs. re-land by wrapper nesting belongs in the follow-up you mentioned.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4352860356","thread_id":"inline:4352860356","verdict_sha256":"32592fcbb473dfabe9d1748a56ce87b90c93808408ef349359525c2b8783cef0"} -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6482340705
Thanks, @dongjoon-hyun. Pushed 3731bcf and f309b6a, which address items 23-27 in this PR, and updated the PR description.
**23**: `update_jira_for_pr` now reads the skip tags with a new `leading_title_tags()`, from the same leading bracket run as the ids, after unwrapping and with aliases normalized, instead of through `Title.parse`. On your question whether the `[MINOR]`/`[TRIVIAL]` subjects should keep updating their ticket: I think they should. `Title.parse` rejects an id next to those markers, so a subject has both only if it predates that rule or the committer added the marker at the component prompt. The older ones were normally merged as the fix for that id; the skip tags only came with d1a7c29232e in 2026, and no subject since then has an id and a marker. For example, SPARK-23960 got fix version 2.4.0 when 271c891b919 `[SPARK-23960][SQL][MINOR] Mark HashAggregateExec.bufVars as transient` was merged, and still has it although e904dfaf0d1 reverted that commit the same day, which is what the revert path is for. So `[FOLLOWUP]` is read from the run unconditionally, and `[MINOR]`/`[TRIVIAL]` in t
he title skip the update only when it has no leading id, which is the same as at 14e286c for every title `Title.parse` accepts. A `[MINOR]` that the committer chooses at the component prompt still skips, since `title_components` is added after the markers are dropped. Replaying this over the 50,753 subjects of master at 1a2450b3e5c, the 17 wrapped subjects whose decision changes against master are all reverts of `[FOLLOW-UP]` changes, now skipped. Counted as you did, over all subjects if reverted: the 21 `[FOLLOWUP]`/`[FOLLOW-UP]` subjects with a leading id are now skipped, the 119 with `[MINOR]`/`[TRIVIAL]` and a leading id still update it, and 193 with a skip tag but no leading id (191 of them `[MINOR]`/`[TRIVIAL]` only, e.g. `[SQL][MINOR] ...`) are now skipped like a direct `[MINOR]` merge. Only one of those 193, `[Minor] [SQL] [SPARK-6729] ...` from 2015, has an id later in the run; it is now skipped rather than offering to revert SPARK-6729 as master did, and I left it so rathe
r than widen the rule for one subject. The doctest covers `[SPARK-1234][SQL][FOLLOWUP][MINOR]` (skipped, `Revert` and `Reapply`), `[MINOR][TRIVIAL][SQL]` (skipped), `[SQL][MINOR]` and `[MINOR][SPARK-1234]` (skipped), and `[SPARK-1234][SQL][MINOR]` / `[SPARK-1234][SQL][TRIVIAL]` (still revert or resolve SPARK-1234).
**24**: `is_followup` now comes from `leading_title_tags(title)`, so a wrapped `[FOLLOWUP]` or `[FOLLOW-UP]` title skips the similarity check like a direct one. For an ordinary title, `title` there is already normalized, so this matches `title_components`. The doctest also checks that `FOLLOWUP` in the title text does not count.
**25**: The id collection, the Epic/Umbrella check, and the match confirmation moved from `main()` into `check_linked_jira_issues(pr_num, title)`, with no other change than item 24. Its doctest mocks `asf_jira` and uses the #59587 title: only `SPARK-60328` is fetched and confirmed, so `SPARK-34261` as an Epic does not block. It also covers confirming two leading ids, aborting when one is declined or the prompt is answered with Enter, a ticket that cannot be fetched (the next id is still checked), a leading Epic and Umbrella both listed before any confirmation, and the `is_followup` cases from item 24. Going back to the whole-title regex in this helper now fails 5 doctests.
**26**: `resolve_jira_issues` now takes `jira_ids`, and `update_jira_for_pr` extracts them once. They stay above the skip check, since the item 23 rule needs them. The resolve-path doctest now also passes two leading ids, and a title without an id.
**27**: Added `Component.normalize(token)`, used by `main()`, `prompt_for_components()`, which had a third copy, and `leading_title_tags()`, which both `update_jira_for_pr` and `check_linked_jira_issues` use.
291 doctests pass, and ruff 0.14.8 `check` / `format --check` pass. All 25 mutants were killed, including reading the skip tags only via `Title.parse` or only for wrapped titles, letting `[MINOR]` skip a title with an id, `is_followup` by raw substring or not inside wrappers, checking or resolving only the first id, not aborting on a declined match or on Enter, stopping at or not skipping an unfetchable ticket, dropping a `[MINOR]` chosen at the prompt, listing only the first blocker, the old regex in `check_linked_jira_issues` or in `update_jira_for_pr`, and an identity `Component.normalize`.
The wrapper-parity follow-up now only needs to decide revert vs. reapply by nesting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59708:
URL: https://github.com/apache/spark/pull/59708#discussion_r4362249962
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
+ while changed:
+ changed = False
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
+ title = title[len(prefix) : -1]
+ changed = True
+ break
+ return title
+
+
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, only ids that
+ appear as leading bracket tags are returned, so a SPARK-NNNNN reference in
+ the title body is not mistaken for the ticket the PR resolves. Any
+ ``Revert "..."`` / ``Reapply "..."`` wrappers are stripped first (including
+ nested ones), so those PRs still yield the wrapped id.
+
+ The title is parsed with :meth:`Title.parse`. When parsing fails -- the
+ title has no recognized leading tag, or starts with ``[SPARK-N]`` but breaks
+ a later rule (e.g. ``[SPARK-1234][SQL][MINOR] Fix``) -- the leading
+ ``[SPARK-N]`` brackets are still collected directly, so a clearly leading id
+ is never dropped. A title with no leading ``[SPARK-N]`` bracket yields an
+ empty list.
+
+ >>> leading_jira_ids("[SPARK-1234][SQL] Follow up to SPARK-5678")
+ ['SPARK-1234']
+ >>> leading_jira_ids('Reapply "[SPARK-1234][SQL] Fix"')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[MINOR][SQL] Fix typo")
+ []
+ >>> leading_jira_ids("No leading tag, mentions SPARK-5678")
+ []
+ """
+ title = _unwrap_title(title)
+ try:
+ return Title.parse(title).jira_ids()
+ except ValueError:
+ return _leading_spark_brackets(title)
+
+
+def _leading_spark_brackets(title: str) -> List[str]:
+ """Collect the run of leading ``[SPARK-N]`` bracket tags from ``title``.
+
+ Used as a fallback when :meth:`Title.parse` rejects a title that still
+ begins with one or more ``[SPARK-N]`` tags. Scans from the start and stops
+ at the first bracket that is not a SPARK id, so a SPARK id in the body is
+ not collected.
+
+ >>> _leading_spark_brackets("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> _leading_spark_brackets("[SPARK-1][SPARK-2][SQL] Fix mentions SPARK-9999")
+ ['SPARK-1', 'SPARK-2']
+ >>> _leading_spark_brackets("[SQL] no leading id")
+ []
+ """
+ ids = []
+ pos = 0
Review Comment:
Minor: `Title.parse` strips the title first (`raw = raw.strip()`, L2119), but this scan starts at position 0 of the unstripped title, so the two paths in `leading_jira_ids` disagree on leading whitespace. At 4c11086, `leading_jira_ids('Revert " [SPARK-1][SQL] Fix"')` returns `['SPARK-1']` because the parse succeeds, but `leading_jira_ids('Revert " [SPARK-1][SQL][MINOR] Fix"')` returns `[]` because the parse fails and the fallback finds no bracket at position 0. Could you strip the title before scanning, as `Title.parse` does?
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
+ while changed:
+ changed = False
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
+ title = title[len(prefix) : -1]
+ changed = True
+ break
+ return title
+
+
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, only ids that
+ appear as leading bracket tags are returned, so a SPARK-NNNNN reference in
+ the title body is not mistaken for the ticket the PR resolves. Any
+ ``Revert "..."`` / ``Reapply "..."`` wrappers are stripped first (including
+ nested ones), so those PRs still yield the wrapped id.
+
+ The title is parsed with :meth:`Title.parse`. When parsing fails -- the
+ title has no recognized leading tag, or starts with ``[SPARK-N]`` but breaks
+ a later rule (e.g. ``[SPARK-1234][SQL][MINOR] Fix``) -- the leading
+ ``[SPARK-N]`` brackets are still collected directly, so a clearly leading id
+ is never dropped. A title with no leading ``[SPARK-N]`` bracket yields an
+ empty list.
+
+ >>> leading_jira_ids("[SPARK-1234][SQL] Follow up to SPARK-5678")
+ ['SPARK-1234']
+ >>> leading_jira_ids('Reapply "[SPARK-1234][SQL] Fix"')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[MINOR][SQL] Fix typo")
+ []
+ >>> leading_jira_ids("No leading tag, mentions SPARK-5678")
+ []
+ """
+ title = _unwrap_title(title)
+ try:
+ return Title.parse(title).jira_ids()
+ except ValueError:
+ return _leading_spark_brackets(title)
Review Comment:
**Wrapped titles whose id is not in a leading bracket still lose it.** The fallback only collects `[SPARK-N]` brackets that start at position 0, so a bare leading id (`SPARK-N: ...`) or a tag before the id (`[WIP] [SPARK-N]`) yields `[]`, while the old `re.findall` found it. There is a recent example in master's history: c8db813644f has the subject `SPARK-49004`, and its revert f15570dbfe1 (2024-08-01) is `Revert "SPARK-49004"`. I ran this helper at 4c11086: `leading_jira_ids('Revert "SPARK-49004"')` returns `[]`, while master's regex returns `['SPARK-49004']`. For such a revert, `main()` skips the Epic/Umbrella blocker and the match prompt, and `update_jira_for_pr` calls `revert_jira_issue(branches, "")`, so pressing Enter skips the fix-version removal. Of the 617 wrapped `Revert "..."` / `Reapply "..."` subjects in master's history, 10 lose their only id this way; the other 9 are from 2014-2015. Could you also accept a bare `SPARK-N` at the start of the unwrapped title, or say i
n the docstring that such titles get no default id?
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
+ while changed:
+ changed = False
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
+ title = title[len(prefix) : -1]
+ changed = True
+ break
+ return title
+
+
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, only ids that
+ appear as leading bracket tags are returned, so a SPARK-NNNNN reference in
+ the title body is not mistaken for the ticket the PR resolves. Any
+ ``Revert "..."`` / ``Reapply "..."`` wrappers are stripped first (including
+ nested ones), so those PRs still yield the wrapped id.
+
+ The title is parsed with :meth:`Title.parse`. When parsing fails -- the
+ title has no recognized leading tag, or starts with ``[SPARK-N]`` but breaks
+ a later rule (e.g. ``[SPARK-1234][SQL][MINOR] Fix``) -- the leading
+ ``[SPARK-N]`` brackets are still collected directly, so a clearly leading id
+ is never dropped. A title with no leading ``[SPARK-N]`` bracket yields an
+ empty list.
+
+ >>> leading_jira_ids("[SPARK-1234][SQL] Follow up to SPARK-5678")
+ ['SPARK-1234']
+ >>> leading_jira_ids('Reapply "[SPARK-1234][SQL] Fix"')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[MINOR][SQL] Fix typo")
+ []
+ >>> leading_jira_ids("No leading tag, mentions SPARK-5678")
+ []
+ """
+ title = _unwrap_title(title)
+ try:
Review Comment:
Nit: for every title that `Title.parse` accepts, `Title.parse(t).jira_ids()` equals `_leading_spark_brackets(t.strip())`. The parse rules require SPARK ids to come before any component and do not let a marker coexist with an id, so the accepted ids are always the first run of `[SPARK-N]` brackets. I compared both on all 50,682 subjects in master's history and found no difference. So this `try`/`except`, and `Title.jira_ids`, whose only caller is this function, are a second path to the same result. Could you use `return _leading_spark_brackets(_unwrap_title(title).strip())` and drop `Title.jira_ids`? That would also fix the whitespace case on L2266.
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
+ while changed:
+ changed = False
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
+ title = title[len(prefix) : -1]
+ changed = True
+ break
+ return title
+
+
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, only ids that
Review Comment:
Nit: my item 9 is still partly open. The `re.findall` comparison it asked to drop was in this docstring (L2187 at 85aa4ba), and it is still here; the reply says it was dropped from `Title.jira_ids`, which never had it. Two of the three `Title.jira_ids` doctests (L2161 and L2165) also still repeat `leading_jira_ids` inputs. Could you drop this sentence and the repeated examples?
##########
dev/merge_spark_pr.py:
##########
@@ -1888,7 +1888,10 @@ def resolve_jira_issues(title, merge_branches, comment, title_components=()):
def update_jira_for_pr(pr_num, title, merge_branches, title_components, is_revert=False):
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
tags = set(title_components)
- jira_title = title[len('Revert "') : -1] if is_revert else title
+ # Revert/Reapply PRs keep their title verbatim, so unwrap before parsing to see the
Review Comment:
Nit: this comment says the title is unwrapped to see the wrapped id, but `jira_title` only feeds the tag set below. The ids come from `leading_jira_ids(title)` on L1894, which gets the still-wrapped `title` and unwraps it again. Could you pass `jira_title` on L1894, or say in the comment that only the markers and components come from `jira_title`?
##########
dev/merge_spark_pr.py:
##########
@@ -2853,6 +2975,16 @@ def main():
... resolver.assert_not_called()
... confirm.assert_not_called()
+ Nested wrappers are unwrapped before the skip-tag and id checks, so an inner
+ skip tag is still honored and an inner id still reaches the revert path.
+
+ >>> update('Revert "Revert "[MINOR][SQL] Fix""', ["master"], is_revert=True)
Review Comment:
Minor: this PR also changes what happens to `Reapply` titles with a skip tag. On master, `update_jira_for_pr` unwrapped only `Revert "`, so `Reapply "[MINOR][SQL] Fix"` reached `resolve_jira_issues`; now it is unwrapped and skipped. I ran `update_jira_for_pr` at 4c11086 with `Reapply "[MINOR][SQL] Fix"` and `Reapply "[SPARK-1234][SQL][FOLLOWUP] Fix"`: neither `resolve_jira_issues` nor `continue_maybe` is called. But the skip-tag doctests (L2969-L2976 and here) use only `Revert "..."` titles with `is_revert=True`, so nothing covers this new path. Could you add the two `Reapply` titles, called without `is_revert`, to the skip-tag loop?
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
+ while changed:
+ changed = False
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
+ title = title[len(prefix) : -1]
+ changed = True
+ break
+ return title
+
+
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, only ids that
+ appear as leading bracket tags are returned, so a SPARK-NNNNN reference in
+ the title body is not mistaken for the ticket the PR resolves. Any
+ ``Revert "..."`` / ``Reapply "..."`` wrappers are stripped first (including
+ nested ones), so those PRs still yield the wrapped id.
+
+ The title is parsed with :meth:`Title.parse`. When parsing fails -- the
+ title has no recognized leading tag, or starts with ``[SPARK-N]`` but breaks
+ a later rule (e.g. ``[SPARK-1234][SQL][MINOR] Fix``) -- the leading
+ ``[SPARK-N]`` brackets are still collected directly, so a clearly leading id
+ is never dropped. A title with no leading ``[SPARK-N]`` bracket yields an
+ empty list.
+
+ >>> leading_jira_ids("[SPARK-1234][SQL] Follow up to SPARK-5678")
+ ['SPARK-1234']
+ >>> leading_jira_ids('Reapply "[SPARK-1234][SQL] Fix"')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[MINOR][SQL] Fix typo")
+ []
+ >>> leading_jira_ids("No leading tag, mentions SPARK-5678")
+ []
+ """
+ title = _unwrap_title(title)
+ try:
+ return Title.parse(title).jira_ids()
+ except ValueError:
+ return _leading_spark_brackets(title)
+
+
+def _leading_spark_brackets(title: str) -> List[str]:
+ """Collect the run of leading ``[SPARK-N]`` bracket tags from ``title``.
+
+ Used as a fallback when :meth:`Title.parse` rejects a title that still
+ begins with one or more ``[SPARK-N]`` tags. Scans from the start and stops
+ at the first bracket that is not a SPARK id, so a SPARK id in the body is
+ not collected.
+
+ >>> _leading_spark_brackets("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> _leading_spark_brackets("[SPARK-1][SPARK-2][SQL] Fix mentions SPARK-9999")
+ ['SPARK-1', 'SPARK-2']
+ >>> _leading_spark_brackets("[SQL] no leading id")
+ []
+ """
+ ids = []
+ pos = 0
+ while pos < len(title):
+ m = _BRACKET_TAG_RE.match(title, pos)
Review Comment:
Nit: this loop copies the bracket scan in `Title.parse` (L2126-L2145): `_BRACKET_TAG_RE.match(raw, pos)`, `.upper()`, `pos = m.end()`, and the inner loop that skips spaces. If `Title.parse` later accepts another separator between tags, this copy keeps the old rule. Could both use one small generator that yields each leading tag?
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
Review Comment:
Nit: the `changed` flag, the `for` loop, and the `break` could be one regex next to the other `_*_RE` constants, e.g. `_WRAPPER_RE = re.compile(r'(?:Revert|Reapply) "(.*)"')` with `while (m := _WRAPPER_RE.fullmatch(title)): title = m.group(1)`. I checked it against the doctests above; only the degenerate `Revert "` case changes, returning the input instead of `''`. Could you simplify it that way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6409402518
Thanks, @dongjoon-hyun and @spark-test. Pushed 14e286c.
**19**: `update_jira_for_pr` now normalizes the components of the unwrapped title through `Component.find` before the skip-tag check, as `main()` does for ordinary titles, so a wrapped `[FOLLOW-UP]` is skipped like `[FOLLOWUP]`. `Revert "[SPARK-55313][PYTHON][FOLLOW-UP] Only add condabin to PATH for pip tests"` (efe9b5335f3) is now skipped, and so is its `Reapply` form; `FOLLOW-UP` is the only alias of a skip tag in the registry. I added `Revert "[SPARK-1234][SQL][FOLLOW-UP] Fix"` and its `Reapply` form to the skip-tag loops, and the description now mentions the alias. Since this PR already extends the skip to `Reapply` and nested titles, I fixed it here rather than in a follow-up.
**20**: `update()` now patches `resolve_jira_issue` instead of `resolve_jira_issues`, and the ordinary-merge loop asserts that `"SPARK-1234"` reaches it for `[SPARK-1234][SQL] Fix mentions SPARK-9999` (the #59587 shape) and for `Reapply "[SPARK-1234][SQL] Fix"`. I also restored `leading_jira_ids('Reapply "[SPARK-1234][SQL] Fix"')` and added `leading_jira_ids('Revert "Revert "[SPARK-1234][SQL] Fix""')`, as @spark-test suggested. I had dropped the first as a repeat of an `_unwrap_title` input, but it was the only `leading_jira_ids` example with a `Reapply` wrapper. I checked the doctests against mutants of the code: an unwrap in `leading_jira_ids` that handles only `Revert "`, or only one layer of either wrapper, a skip-tag check without alias normalization, the old whole-title regex in `resolve_jira_issues`, and a one-layer `_unwrap_title` each make at least one doctest fail.
**21**: The `Revert "SPARK-49004"` case now has its own sentence, and the opening sentence says "each leading JIRA id".
**22**: Switched to the builtin `tuple`; the `typing` import is now `Iterator, List, Optional`.
All 247 doctests pass, and `ruff check` / `ruff format --check` are clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59708:
URL: https://github.com/apache/spark/pull/59708#discussion_r4419144117
##########
dev/merge_spark_pr.py:
##########
@@ -1905,14 +1971,17 @@ def update_jira_for_pr(pr_num, title, merge_branches, title_components, is_rever
return
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
- tags = set(title_components)
- jira_title = title[len('Revert "') : -1] if is_revert else title
- try:
- parsed = Title.parse(jira_title)
- tags.update(parsed.leading)
- tags.update(parsed.components)
- except ValueError:
- pass
+ jira_ids = leading_jira_ids(title)
+ # Revert/Reapply PRs keep their title verbatim, so the tags are read inside the wrappers,
+ # without Title.parse, which rejects some older titles such as [SPARK-N][FOLLOWUP][MINOR].
+ tags = leading_title_tags(title)
+ if jira_ids:
Review Comment:
**A revert or reapply of a commit whose `[MINOR]` was entered at the component prompt updates the ticket that its merge skipped.** The comment above names two sources of an id next to a marker, older subjects and the component prompt, but only the first one is merged as the fix for that id. The prompt produces exactly the subject this rule updates:
1. Merging `[SPARK-1234] Fix`, which has no primary component, the committer enters `SQL,MINOR` at the prompt (L2662). `main()` normalizes the title to `[SPARK-1234][SQL][MINOR] Fix` (L2686-L2688), commits it with that subject (L858), and skips the JIRA update because `title_components` has `MINOR` (L1984). This is the doctest at L3080.
2. Reverting that commit opens `Revert "[SPARK-1234][SQL][MINOR] Fix"`. Its `title_components` is `[]` (L2643), so L1982 drops `MINOR` and the script calls `revert_jira_issue(..., "SPARK-1234")`. This is the doctest at L3078, with the same subject.
So the revert offers to remove the unreleased fix versions of SPARK-1234 for its branches and to reopen the ticket, although this merge never added them; they come from the change that resolved SPARK-1234. A `Reapply` of the same commit reaches `resolve_jira_issue` and offers to add fix versions. As the PR description says, no subject in master's history since the skip tags were added has this shape yet, but every future merge that adds `MINOR` or `TRIVIAL` at the prompt to a title with an id produces one. Item 29 suggests a way to remove this source.
##########
dev/merge_spark_pr.py:
##########
@@ -2467,7 +2657,7 @@ def main():
c = Component.find(tag)
if c is not None and c.primary:
has_primary = True
- components.append(c.tag if c is not None else tag)
+ components.append(Component.normalize(tag))
if not has_primary:
new_tags = prompt_for_components()
Review Comment:
**`[MINOR]` or `[TRIVIAL]` entered at the component prompt always yields a title that `Title.parse` rejects.** `Title.parse` requires the title to open with an id, `[MINOR]`, or `[TRIVIAL]` (L2255), so `parsed.leading` is never empty here, and it rejects a marker next to an id or a second marker (L2275-L2276). So `[SPARK-1234] Fix` plus `SQL,MINOR` at this prompt becomes `[SPARK-1234][SQL][MINOR] Fix`, and `[MINOR] Fix` plus `SQL,TRIVIAL` becomes `[MINOR][SQL][TRIVIAL] Fix`. Typed as the PR title, either one fails at L2651. This is the only way a new subject gets an id next to a marker, which is what makes item 28 possible. The marker also lives only in this run's `title_components`, so a later run cannot recover it from the subject. For example, a backport run of the same PR, whose GitHub title still has no primary component, prompts again and adds fix versions if the committer leaves `MINOR` out this time.
Could the prompt reject `MINOR` and `TRIVIAL`, as it already rejects a list without a primary component? Then an id next to a marker in a subject would mean that the subject predates the rule, the comment at L1979-L1981 would hold as written, the case at L3080 would go away, and `tags |= set(title_components)` at L1984 would no longer have a marker to bring back.
##########
dev/merge_spark_pr.py:
##########
@@ -2916,24 +3050,89 @@ def main():
>>> update('Revert "Fix"', ["master"], is_revert=True)
>>> reverter.assert_called_once_with(["master"], "")
- Wrapped skip tags preserve the existing policy for MINOR, TRIVIAL, and FOLLOWUP changes.
+ A bare leading id, as in the old subject SPARK-49004, still reaches the revert path.
+
+ >>> update('Revert "SPARK-49004"', ["master"], is_revert=True)
+ >>> reverter.assert_called_once_with(["master"], "SPARK-49004")
+
+ Wrapped skip tags follow the same policy as direct merges for MINOR, TRIVIAL, and FOLLOWUP
Review Comment:
Nit: with the rule at L1978-L1982, this sentence no longer holds for an id next to `MINOR` or `TRIVIAL`. The doctests at L3078 and L3080 pass the same subject, `[SPARK-1234][SQL][MINOR] Fix`, and the wrapped one is updated while the direct one is skipped. Could it say that wrapped tags follow the direct-merge policy for `FOLLOWUP`, and for `MINOR` and `TRIVIAL` only when there is no leading id? If item 29 is taken, the case at L3080 can go, and the paragraph at L3073-L3076 can describe only the older subjects.
##########
dev/merge_spark_pr.py:
##########
@@ -2001,6 +2069,16 @@ def find(cls, token):
return c
return None
+ @classmethod
+ def normalize(cls, token):
+ """Return the registered tag for ``token``, or ``token`` itself if none matches.
+
+ >>> Component.normalize("FOLLOW-UP"), Component.normalize("BOGUS")
+ ('FOLLOWUP', 'BOGUS')
+ """
+ c = cls.find(token)
+ return c.tag if c is not None else token
Review Comment:
Nit: `find` matches on `token.strip().upper()` (L2066), but on a miss this returns `token` unchanged, so `Component.normalize(" docs ")` is `'DOC'` while `Component.normalize(" bogus ")` is `' bogus '`. All three callers pass a stripped, upper-cased token today, so nothing breaks. Since the docstring describes a general helper, could a miss return `token.strip().upper()` as well, or could the docstring say that the token is expected to be normalized already?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6512272900
Thanks, @dongjoon-hyun. Pushed 8523320.
**28, 29**: You're right; I missed that the merge and a later revert of the same subject disagree. `prompt_for_components()` now rejects `[MINOR]` and `[TRIVIAL]` with a message that a PR without a JIRA ticket puts them at the start of its title, and asks again, as it already does for a list without a primary component. So an id next to a marker in a subject now only comes from a subject that predates the rule or was not merged with this script, and the comment in `update_jira_for_pr` says that. With no marker left for it to bring back, I removed `tags |= set(title_components)` and its doctest, so the skip check reads only the title and agrees with a later revert of the same subject. That needs every prompted tag to be readable by the bracket scan: a token like `SPARK CORE` was passed through with only an unknown-tag warning, and since `main()` moves `[FOLLOWUP]` to the end, `[SPARK-1234][SQL][SPARK CORE][FOLLOWUP] Fix` would hide the `[FOLLOWUP]` from the scan. So the prompt now
also rejects such tokens, and SPARK ids, which became an extra leading id when entered first. On the backport run you mention: the markers can no longer come from the prompt, so both runs read them from the same GitHub title. Other prompted components, including `[FOLLOWUP]`, can still differ between the two runs, as on master; each run's commit and its later revert agree. A new `prompt_for_components` doctest feeds `sql,minor`, ` Trivial `, `sql, spark core`, `sql,[tests]`, `sql,spark-2`, `sql][minor`, and `tests` before `sql, follow-up`, and checks that the others are rejected and the last returns `['SQL', 'FOLLOWUP']`. The direct-merge skip loop now also has `[SPARK-1234][SQL][FOLLOWUP] Fix`. The description has a paragraph for this.
**30**: The doctest now says that wrapped tags follow the direct-merge policy for `FOLLOWUP`, and for `MINOR` and `TRIVIAL` when the title has no leading id. The paragraph after it now describes only older subjects.
**31**: On a miss, `Component.normalize()` now returns `token.strip().upper()`, and the doctest uses ` follow-up ` and ` bogus `.
335 doctests pass, and ruff 0.14.8 `check` / `format --check` pass. All 34 mutants were killed, including accepting both markers at the prompt, rejecting only `[MINOR]` there, accepting an unreadable or partly readable token or a SPARK id there, not skipping a direct `[FOLLOWUP]` merge, checking the markers only when no primary component is given, and returning the raw token from `Component.normalize()` on a miss.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59708:
URL: https://github.com/apache/spark/pull/59708#discussion_r4371925624
##########
dev/merge_spark_pr.py:
##########
@@ -2853,6 +2949,26 @@ def main():
... resolver.assert_not_called()
... confirm.assert_not_called()
+ Reapply titles are unwrapped too, so their skip tags apply as well.
+
+ >>> for title in ('Reapply "[MINOR][SQL] Fix"', 'Reapply "[SPARK-1234][SQL][FOLLOWUP] Fix"'):
+ ... update(title, ["master"])
+ ... reverter.assert_not_called()
+ ... resolver.assert_not_called()
+ ... confirm.assert_not_called()
+
+ Nested wrappers are unwrapped before the skip-tag and id checks, so an inner
+ skip tag is still honored and an inner id still reaches the revert path. A
+ revert of a reapply is used here because it is a revert both by its outermost
+ wrapper, which is how main() sets is_revert today, and by wrapper nesting.
+
+ >>> update('Revert "Reapply "[MINOR][SQL] Fix""', ["master"], is_revert=True)
+ >>> reverter.assert_not_called()
+ >>> resolver.assert_not_called()
+ >>> confirm.assert_not_called()
+ >>> update('Revert "Reapply "[SPARK-1234][SQL] Fix""', ["master"], is_revert=True)
+ >>> reverter.assert_called_once_with(["master"], "SPARK-1234")
+
Ordinary merges and reapplies still resolve the issue or add fix versions.
>>> for title in ('[SPARK-1234][SQL] Fix', 'Reapply "[SPARK-1234][SQL] Fix"'):
Review Comment:
Minor: no doctest now checks the id that `resolve_jira_issues` extracts (L1871), which is where my item 1 regressed. This loop mocks `resolve_jira_issues` as a whole (L2922-L2923), so it only checks that the wrapped title is passed on. The `leading_jira_ids('Reapply "[SPARK-1234][SQL] Fix"')` doctest added for my item 5 was dropped in 205a17f with the examples that repeat `_unwrap_title` inputs. And since my item 15, L1894 passes the already-unwrapped `jira_title`, so the `update_jira_for_revert` cases no longer reach the unwrap in `leading_jira_ids` with a wrapper either. As a result, if L2241 unwrapped only `Revert "..."` (or only one layer), every doctest would still pass, while a `Reapply "[SPARK-1234][SQL] Fix"` PR would again skip the Epic/Umbrella blocker and the match prompt in `main()` and get `Enter a JIRA id []:` on merge. The #59587 case, a body id on an ordinary merge, is not checked on this path either. Could you patch `resolve_jira_issue` instead of `resolve_jira_is
sues` in `update()`, and assert for both titles here that `"SPARK-1234"` reaches it, e.g. `resolver.assert_called_once_with(["master"], "Issue resolved by pull request 123\\n[https://github.com/apache/spark/pull/123]", "SPARK-1234", title_components=[])`? A body reference such as `Fix mentions SPARK-9999` in the first title would cover #59587 as well. Restoring the dropped example would be the minimal alternative.
##########
dev/merge_spark_pr.py:
##########
@@ -1888,7 +1888,10 @@ def resolve_jira_issues(title, merge_branches, comment, title_components=()):
def update_jira_for_pr(pr_num, title, merge_branches, title_components, is_revert=False):
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
tags = set(title_components)
- jira_title = title[len('Revert "') : -1] if is_revert else title
+ # Revert/Reapply PRs keep their title verbatim, so unwrap before parsing to see the
+ # wrapped id, markers, and components.
+ jira_title = _unwrap_title(title)
+ jira_ids = leading_jira_ids(jira_title)
try:
parsed = Title.parse(jira_title)
tags.update(parsed.leading)
Review Comment:
**A wrapped `[FOLLOW-UP]` is not skipped.** `[FOLLOW-UP]` is a registered alias of FOLLOWUP (`Component("FOLLOWUP", ("FOLLOW-UP",))`, L2018), and `main()` maps it to FOLLOWUP for ordinary titles (L2487-L2491). A Revert/Reapply title keeps `title_components == []`, though, and the components added on L1898 are compared raw against `skip_jira_title_tags` (L1901). With this code, #54151's title, `Revert "[SPARK-55313][PYTHON][FOLLOW-UP] Only add condabin to PATH for pip tests"`, would not be skipped: the script asks to update JIRA and calls `revert_jira_issue(..., "SPARK-55313")`, which offers to remove the parent ticket's fix versions and to reopen it, although only the follow-up was reverted. The same title with `[FOLLOWUP]` is skipped, and so is a direct merge of the `[FOLLOW-UP]` title. For `Revert` this predates this PR (SPARK-59808). But this PR extends the skip to `Reapply` and nested titles, and the description says that a `Reapply` or a revert of a revert of a `[FOLLOWUP]` c
hange now skips the JIRA update, while `Reapply "...[FOLLOW-UP]..."` still offers to add fix versions to the parent ticket. Titles have been normalized at merge time since SPARK-56979, but 17 of the 617 wrapped subjects in master's history use `[FOLLOW-UP]` (22 use `[FOLLOWUP]`), and about 1,200 older commits carry it. Could you normalize the components on L1898 as `main()` does, e.g. `tags.update(c.tag if (c := Component.find(t)) else t for t in parsed.components)`, and add `'Revert "[SPARK-1234][SQL][FOLLOW-UP] Fix"'` to the skip-tag loop? A follow-up would also be fine, since `Revert` titles already behave this way on master.
##########
dev/merge_spark_pr.py:
##########
@@ -2023,6 +2025,28 @@ def find(cls, token):
_SPARK_ID_RE = re.compile(r"^SPARK-\d+$", re.IGNORECASE)
_VERSION_TAG_RE = re.compile(r"^\d+\.(\d+|X)$")
_LEADING_TAGS = frozenset({"MINOR", "TRIVIAL"})
+# A SPARK id that opens a title without brackets, e.g. the old subject "SPARK-49004".
+_BARE_SPARK_ID_RE = re.compile(r"SPARK-[0-9]+\b", re.IGNORECASE | re.ASCII)
+# A Revert "..." / Reapply "..." wrapper, which main() keeps verbatim in the PR title.
+_WRAPPER_RE = re.compile(r'(?:Revert|Reapply) "(.*)"', re.DOTALL)
+
+
+def _iter_bracket_tags(raw: str) -> Iterator[Tuple[str, int]]:
Review Comment:
Nit: `Tuple` is used only here. `Iterator[tuple[str, int]]` would match `parse_version`'s `Optional[tuple[int, int, int]]` (L140), the file's only other tuple annotation, and keep the import on L47 as `from typing import Iterator, List, Optional`.
##########
dev/merge_spark_pr.py:
##########
@@ -2843,6 +2937,8 @@ def main():
>>> reverter.assert_called_once_with(["branch-4.2"], "SPARK-1234")
>>> update('Revert "Fix"', ["master"], is_revert=True)
>>> reverter.assert_called_once_with(["master"], "")
+ >>> update('Revert "SPARK-49004"', ["master"], is_revert=True)
Review Comment:
Nit: this case runs on `master` and finds its id, so neither sentence of the paragraph above it (L2934, "Backport-only runs use the same revert path. A missing JIRA id is entered interactively.") describes it. Could you add a sentence for it, e.g. "A bare leading id, as in the old subject SPARK-49004, still reaches the revert path."? While here, the opening sentence on L2911, "Reverts update each referenced JIRA once ...", no longer fits the first case, where the referenced SPARK-9999 is deliberately skipped; "each leading JIRA id" would.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "spark-test (via GitHub)" <gi...@apache.org>.
spark-test commented on code in PR #59708:
URL: https://github.com/apache/spark/pull/59708#discussion_r4352860356
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,92 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip a ``Revert "..."`` / ``Reapply "..."`` wrapper from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping here
+ lets the id-extraction and skip-tag logic see them. A title without such a
+ wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ """
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
Review Comment:
**Nit (P3):** This strips only one wrapper. `main()` still treats a revert-of-revert title such as `Revert "Revert "[SPARK-1234][SQL] Fix""` as a revert PR (it starts with `Revert "` and ends with `"`), but after one unwrap the inner text still begins with `Revert`, so `Title.parse` raises and `_leading_spark_brackets` finds no bracket at position 0. I ran the helpers at 2233a6b: `leading_jira_ids` returns `[]` for this title and for `Revert "Reapply "[SPARK-1234][SQL] Fix""`. On master, `re.findall` found `SPARK-1234`. As a result, `main()` skips the Epic/Umbrella blocker and the JIRA-match confirmation for these PRs, and `update_jira_for_pr` calls `revert_jira_issue` with an empty default id (and the skip-tag check misses inner `[MINOR]`/`[FOLLOWUP]`). Could you keep unwrapping in a loop until no `Revert "..."`/`Reapply "..."` wrapper remains, and add a doctest for the nested case?
This extends existing discussions [1](https://github.com/apache/spark/pull/59708#issuecomment-6343253163), [2](https://github.com/apache/spark/pull/59708#pullrequestreview-5615544014).
**Verification:**
- **Regression:** A doctest shows that a revert-of-revert title yields the inner leading SPARK id from leading_jira_ids, and that _unwrap_title returns the innermost title.
- **Compatibility:** The existing doctests for single-level Revert/Reapply titles, parse-failure fallback titles, and body-id exclusion still pass unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59708:
URL: https://github.com/apache/spark/pull/59708#discussion_r4405948144
##########
dev/merge_spark_pr.py:
##########
@@ -1888,11 +1888,17 @@ def resolve_jira_issues(title, merge_branches, comment, title_components=()):
def update_jira_for_pr(pr_num, title, merge_branches, title_components, is_revert=False):
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
tags = set(title_components)
- jira_title = title[len('Revert "') : -1] if is_revert else title
+ # Revert/Reapply PRs keep their title verbatim, so unwrap before parsing to see the
+ # wrapped id, markers, and components.
+ jira_title = _unwrap_title(title)
+ jira_ids = leading_jira_ids(jira_title)
try:
parsed = Title.parse(jira_title)
Review Comment:
**A wrapped `[FOLLOWUP]` is still not skipped when `Title.parse` rejects the inner title.** The ids come from the bracket run without validation (L2253-L2256), but the skip tags come only from `Title.parse` (L1895-L1903), and on a `ValueError` the tag set stays empty. `Title.parse` rejects a `[MINOR]` or `[TRIVIAL]` next to an id, which older subjects often have. Replaying both rules over master's history, 140 subjects open with `[SPARK-N]`, have a skip tag in that bracket run, and are rejected by `Title.parse`. 21 of them are `[FOLLOWUP]`/`[FOLLOW-UP]`, e.g.:
- 09efee759b6 (2025-07-15) `[SPARK-52686][FOLLOWUP][SQL][MINOR][TESTS] Fix a failing test`
- 73a3abae48c (2025-03-30) `[SPARK-51612][MINOR][FOLLOW-UP][SQL] Update Desc As JSON test to use withSQLConf`
For `Revert "[SPARK-52686][FOLLOWUP][SQL][MINOR][TESTS] Fix a failing test"`, the script asks to update JIRA and calls `revert_jira_issue(..., "SPARK-52686")`, which offers to remove the fix versions of SPARK-52686 and to reopen it, although only the `[FOLLOWUP]` change was reverted. The `Reapply` form reaches `resolve_jira_issue` with the same id. This is the case of item 19 for another reason, while the doctest text at L2953-L2954 and the PR description say that wrapped titles now follow the direct-merge skip policy.
Could the tags come from the same bracket run as the ids? For example, in place of the `try`/`except`:
```python
for tag, _ in _iter_bracket_tags(jira_title):
c = Component.find(tag)
tags.add(c.tag if c is not None else tag)
```
For a title that `Title.parse` accepts, every tag in the run goes to `leading` or `components`, so the set is the same. This would also skip the other 119 subjects, which pair `[MINOR]` or `[TRIVIAL]` with an id. If those should keep updating their ticket, only `FOLLOWUP` could be taken from the run. A title such as `Revert "[SPARK-1234][SQL][FOLLOWUP][MINOR] Fix"` in the skip-tag loop at L2956-L2962 would cover it.
##########
dev/merge_spark_pr.py:
##########
@@ -2546,7 +2647,7 @@ def main():
continue_maybe(msg)
# asf_jira is guaranteed to be set here: initialize_jira() fails fast otherwise.
- jira_ids = re.findall("SPARK-[0-9]{4,5}", title)
+ jira_ids = leading_jira_ids(title)
Review Comment:
Minor: the ids found here also feed the JIRA match check below, which takes `is_followup` from `title_components` (L2684). For a `Revert`/`Reapply` PR, `title_components` stays `[]` (L2481), so a wrapped `[FOLLOWUP]` or `[FOLLOW-UP]` title is scored like an ordinary one. Take efe9b5335f3, `Revert "[SPARK-55313][PYTHON][FOLLOW-UP] Only add condabin to PATH for pip tests"`. Its `title_similarity` against the title of the original change, 14bc85248f2 `[SPARK-55313][PYTHON] Do not activate conda environment when installing conda`, is 0.00. With a JIRA summary like that, the committer sees "WARNING: low title similarity, wrong ticket?" and has to confirm SPARK-55313, and then `update_jira_for_pr` skips the JIRA update because of the same tag. A direct `[FOLLOW-UP]` PR gets "(FOLLOWUP: title intentionally differs, not checked)" instead. This already happens on master, but since this PR makes `update_jira_for_pr` read the wrapped tags, could `is_followup` use the same normalized tags of
the unwrapped title? (Posted here because L2684 is outside the diff.)
##########
dev/merge_spark_pr.py:
##########
@@ -1888,11 +1888,17 @@ def resolve_jira_issues(title, merge_branches, comment, title_components=()):
def update_jira_for_pr(pr_num, title, merge_branches, title_components, is_revert=False):
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
tags = set(title_components)
- jira_title = title[len('Revert "') : -1] if is_revert else title
+ # Revert/Reapply PRs keep their title verbatim, so unwrap before parsing to see the
+ # wrapped id, markers, and components.
+ jira_title = _unwrap_title(title)
+ jira_ids = leading_jira_ids(jira_title)
try:
parsed = Title.parse(jira_title)
tags.update(parsed.leading)
- tags.update(parsed.components)
+ # Normalize aliases such as FOLLOW-UP, as main() does for ordinary titles.
+ for tag in parsed.components:
Review Comment:
Nit: this loop repeats the alias normalization in `main()` (L2494-L2498): `Component.find(tag)`, then `c.tag if c is not None else tag`. Item 19 came from these two places disagreeing on aliases, so a small helper that both call, e.g. a `Component.normalize(tag)` classmethod that returns the registered tag or the token itself, would keep them in sync.
##########
dev/merge_spark_pr.py:
##########
@@ -1888,11 +1888,17 @@ def resolve_jira_issues(title, merge_branches, comment, title_components=()):
def update_jira_for_pr(pr_num, title, merge_branches, title_components, is_revert=False):
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
tags = set(title_components)
- jira_title = title[len('Revert "') : -1] if is_revert else title
+ # Revert/Reapply PRs keep their title verbatim, so unwrap before parsing to see the
+ # wrapped id, markers, and components.
+ jira_title = _unwrap_title(title)
+ jira_ids = leading_jira_ids(jira_title)
Review Comment:
Nit: these ids are used only on the revert path (L1917). The resolve path passes the still-wrapped `title` to `resolve_jira_issues` (L1926), which unwraps and scans it again (L1871). Since this is the only caller of `resolve_jira_issues`, could it take `jira_ids` instead of `title`, so both paths use the ids extracted here? This line could then also move below the skip check (L1904-L1911), next to where the ids are used.
##########
dev/merge_spark_pr.py:
##########
@@ -2844,24 +2945,53 @@ def main():
>>> update('Revert "Fix"', ["master"], is_revert=True)
>>> reverter.assert_called_once_with(["master"], "")
- Wrapped skip tags preserve the existing policy for MINOR, TRIVIAL, and FOLLOWUP changes.
+ A bare leading id, as in the old subject SPARK-49004, still reaches the revert path.
+
+ >>> update('Revert "SPARK-49004"', ["master"], is_revert=True)
+ >>> reverter.assert_called_once_with(["master"], "SPARK-49004")
+
+ Wrapped skip tags follow the same policy as direct merges for MINOR, TRIVIAL, and FOLLOWUP
+ changes, including the FOLLOW-UP alias.
>>> for title in ('Revert "[MINOR][SQL] Fix"', 'Revert "[TRIVIAL][SQL] Fix"',
- ... 'Revert "[SPARK-1234][SQL][FOLLOWUP] Fix"'):
+ ... 'Revert "[SPARK-1234][SQL][FOLLOWUP] Fix"',
+ ... 'Revert "[SPARK-1234][SQL][FOLLOW-UP] Fix"'):
... update(title, ["master"], is_revert=True)
... reverter.assert_not_called()
... resolver.assert_not_called()
... confirm.assert_not_called()
- Ordinary merges and reapplies still resolve the issue or add fix versions.
+ Reapply titles are unwrapped too, so their skip tags apply as well.
+
+ >>> for title in ('Reapply "[MINOR][SQL] Fix"', 'Reapply "[SPARK-1234][SQL][FOLLOWUP] Fix"',
+ ... 'Reapply "[SPARK-1234][SQL][FOLLOW-UP] Fix"'):
+ ... update(title, ["master"])
+ ... reverter.assert_not_called()
+ ... resolver.assert_not_called()
+ ... confirm.assert_not_called()
+
+ Nested wrappers are unwrapped before the skip-tag and id checks, so an inner
+ skip tag is still honored and an inner id still reaches the revert path. A
+ revert of a reapply is used here because it is a revert both by its outermost
+ wrapper, which is how main() sets is_revert today, and by wrapper nesting.
+
+ >>> update('Revert "Reapply "[MINOR][SQL] Fix""', ["master"], is_revert=True)
+ >>> reverter.assert_not_called()
+ >>> resolver.assert_not_called()
+ >>> confirm.assert_not_called()
+ >>> update('Revert "Reapply "[SPARK-1234][SQL] Fix""', ["master"], is_revert=True)
+ >>> reverter.assert_called_once_with(["master"], "SPARK-1234")
+
+ Ordinary merges and reapplies resolve the leading id, not an id in the title body.
- >>> for title in ('[SPARK-1234][SQL] Fix', 'Reapply "[SPARK-1234][SQL] Fix"'):
+ >>> for title in ('[SPARK-1234][SQL] Fix mentions SPARK-9999',
Review Comment:
Minor: this loop now pins the id that `resolve_jira_issues` extracts, but nothing reaches the third call site, `jira_ids = leading_jira_ids(title)` in `main()` (L2650), because no doctest calls `main()`. So replacing that call with the old `re.findall("SPARK-[0-9]{4,5}", title)` still passes every doctest, and the list of mutants in the PR description does not include it. With the title from the PR description, `[SPARK-60328][SQL][TESTS] Use a unique jar copy in the HiveDDLSuite SPARK-34261 test`, that would bring back the prompt to confirm SPARK-34261, and an Epic mentioned in a title body would block the merge again. Could the id collection and the Epic/Umbrella check at L2650-L2675 move into a small helper with a doctest that uses this title and a mocked `asf_jira`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6510580733
Thank you, @dongjoon-hyun. I've resolved the conflict.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #59708: [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py
URL: https://github.com/apache/spark/pull/59708
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "spark-test (via GitHub)" <gi...@apache.org>.
spark-test commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6349922671
Thanks, the fixes for items 1-3 look right on 2233a6b. Parsing twice in update_jira_for_pr is cheap and keeps unwrap and fallback in leading_jira_ids, so there is no need to refactor. One small gap remains: _unwrap_title strips only one wrapper level, so a revert-of-revert title still loses its id. See the comment on dev/merge_spark_pr.py.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"conversation:6343253163","thread_id":"conversation:6343253163","verdict_sha256":"15bf7ee0016037bed6132d23d99ae13a35a6805003291c36e9b3ae1e1d6a08a6"} -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #59708:
URL: https://github.com/apache/spark/pull/59708#discussion_r4353465817
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,92 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip a ``Revert "..."`` / ``Reapply "..."`` wrapper from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping here
+ lets the id-extraction and skip-tag logic see them. A title without such a
+ wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ """
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
Review Comment:
Good catch, thanks. Fixed in 4c1108615c1: `_unwrap_title` now strips wrappers in a loop until none remains, so a revert-of-revert such as `Revert "Revert "[SPARK-1234][SQL] Fix""` (and mixed `Revert`/`Reapply` nesting) reduces to the innermost title and the inner id is found again. Added doctests for the nested, mixed, and only-wrapper cases on `_unwrap_title`/`leading_jira_ids`, plus nested skip-tag and nested-id revert-path cases in the `update_jira_for_revert` doctest.
One scoping note: a nested wrapper can also flip the net direction (`Revert "Revert "X""` is a re-land, but `main()` still classifies it as a revert via `is_revert_pr`), so `update_jira_for_pr` would take the revert path rather than resolving. That is a pre-existing bug in the `is_revert_pr`/`is_reapply_pr` classification in `main()`, separate from id extraction and outside this PR's scope. I'll file a follow-up JIRA to decide direction by wrapper parity. This PR keeps its scope to extracting the correct JIRA id.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60468][INFRA] Only treat leading SPARK JIRA tags as JIRA IDs in merge_spark_pr.py [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #59708:
URL: https://github.com/apache/spark/pull/59708#issuecomment-6371016412
Thanks, @dongjoon-hyun. Pushed 205a17f. And thanks for catching item 9: my previous reply was wrong on both counts. The `re.findall` sentence was in the `leading_jira_ids` docstring, not in `Title.jira_ids`, and I had left it there; and two of the `Title.jira_ids` doctests still repeated `leading_jira_ids` inputs even though I said the overlapping examples were dropped.
**10, 11, 16**: `leading_jira_ids` is now a single path: unwrap, then the run of leading `[SPARK-N]` brackets, or a bare leading `SPARK-N` when the title opens without a bracket. `Title.jira_ids` and the `try`/`except` are gone, and I folded the bracket scan into `leading_jira_ids` instead of keeping `_leading_spark_brackets`, since it is the only caller. So `Revert "SPARK-49004"` yields `SPARK-49004` again, and `Revert " [SPARK-1][SQL][MINOR] Fix"` yields `SPARK-1`. Only the first bare id is used (`SPARK-9545, SPARK-9547: ...` yields `SPARK-9545`); no wrapped subject in master's history has more than one bare id. A title whose id follows another tag still yields `[]`, and the docstring now says so.
I re-ran the comparison on master at 67b62cb4081 (50,656 subjects from `git log --format=%s`). For every title that `Title.parse` accepts, the new path matches the parsed leading ids (0 differences). Of the 10 wrapped subjects you found losing their only id at 4c11086, 9 now get it back. The remaining one is `Revert "[WIP] [SPARK-3996]: Shade Jetty in Spark deliverables"` (2014), and `main()` rejects `[WIP]` titles before reaching the JIRA code anyway. Compared with 4c11086, the only other changes are subjects that open with a bare id and now yield it; nothing loses an id.
**12**: Added `Reapply "[MINOR][SQL] Fix"` and `Reapply "[SPARK-1234][SQL][FOLLOWUP] Fix"`, called without `is_revert`, asserting that neither `resolve_jira_issues` nor `continue_maybe` is called. Also added a `Revert "SPARK-49004"` case asserting that the id reaches `revert_jira_issue`.
**13**: Updated the PR description to cover all rounds, including the new skip for `Reapply` and nested wrapped titles.
**14**: Dropped the `re.findall` sentence; the repeated examples went away with `Title.jira_ids`, and I also dropped the `leading_jira_ids` examples that only repeated `_unwrap_title` inputs.
**15**: `update_jira_for_pr` now passes `jira_title` to `leading_jira_ids`.
**17**: `Title.parse` and `leading_jira_ids` now share a small generator, `_iter_bracket_tags`, that yields each leading tag and the position after it and the spaces that follow.
**18**: `_unwrap_title` is now a `_WRAPPER_RE.fullmatch` loop. The degenerate `Revert "` now comes back unchanged, and a doctest records that.
While here, I also made a few edge cases consistent: the bare id accepts the same ASCII digits and case as the bracket path (`[0-9]`, case-insensitive, upper-cased), `_WRAPPER_RE` uses `re.DOTALL` so a newline in the title does not stop the unwrap, as with the old slicing, and `_unwrap_title` strips whitespace at every layer, so `Revert " Revert "[SPARK-1][SQL] Fix""` also yields its id. All 244 doctests pass, and `ruff check` / `ruff format --check` (0.14.8) are clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org