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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
