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: [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]
