damccorm commented on code in PR #40399:
URL: https://github.com/apache/beam/pull/40399#discussion_r4186639469
##########
sdks/python/apache_beam/transforms/util.py:
##########
@@ -2234,8 +2234,8 @@ def find_all(pcoll, regex, group=0, outputEmpty=True):
def _process(element):
matches = regex.finditer(element)
if group == Regex.ALL:
- yield [(m.group(), m.groups()[0]) for m in matches
- if outputEmpty or m.groups()[0]]
+ yield [(m.group(), *m.groups()) for m in matches
+ if outputEmpty or any(m.groups() or (m.group(), ))]
Review Comment:
With `any(m.groups() or (m.group(), ))`, if a pattern has multiple capturing
groups and one group is empty while another is non-empty (for example,
`r'(\w)=(\d?)'` matching `'c='` in your test, where `m.groups()` is `('c',
'')`), `outputEmpty=False` will still include `('c=', 'c', '')` because `'c'`
is non-empty.
Compare this with the existing single-group `CheckAllNonEmptyGroups` test
(`'a(b*)'` on `'abb ax abbb'` with `outputEmpty=False`), where `'ax'` matches
`'a'` with an empty group 1 (`('a', '')`) and is filtered out. With `any(...)`,
simply wrapping `'a'` in a capturing group (`'(a)(b*)'`) would cause `('a',
'a', '')` to be kept despite group 2 being empty.
Should this use `all(m.groups() or (m.group(), ))` instead so
`outputEmpty=False` filters out matches where any captured group is empty
(while behaving identically for 0-group and 1-group patterns)?
```suggestion
yield [(m.group(), *m.groups()) for m in matches
if outputEmpty or all(m.groups() or (m.group(), ))]
```
##########
sdks/python/apache_beam/transforms/util_test.py:
##########
@@ -2678,6 +2678,21 @@ def test_find_all_groups(self):
[('abb', 'bb'), ('abbb', 'bbb')]]),
label='CheckAllNonEmptyGroups')
+ def test_find_all_groups_multiple_and_no_groups(self):
+ with TestPipeline() as p:
+ pcol = (p | beam.Create(['a=1 b=2 c=']))
+ assert_that(
+ pcol
+ | 'two groups' >> util.Regex.find_all(r'(\w)=(\d?)', util.Regex.ALL),
+ equal_to([[('a=1', 'a', '1'), ('b=2', 'b', '2'), ('c=', 'c', '')]]),
+ label='CheckTwoGroups')
+
+ assert_that(
+ pcol | 'no groups' >> util.Regex.find_all(
+ r'\w=\d', util.Regex.ALL, outputEmpty=False),
+ equal_to([[('a=1', ), ('b=2', )]]),
+ label='CheckNoGroups')
Review Comment:
Two small test suggestions here:
1. Could we also add an assertion for `r'(\w)=(\d?)'` with
`outputEmpty=False` (expecting `[[('a=1', 'a', '1'), ('b=2', 'b', '2')]]` if
using `all(...)`) so we test `outputEmpty=False` with multiple groups?
2. In `'no groups'`, `r'\w=\d'` cannot match an empty string anyway (`'c='`
simply doesn't match the pattern, so `outputEmpty=True` and `outputEmpty=False`
return the same result). Using a pattern that can match an empty string (or
testing both `outputEmpty=True` and `outputEmpty=False` on e.g. `r'\d*'`) would
directly exercise the empty-match filtering when `m.groups()` is `()`.
--
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]
