MaxFreedomPollard opened a new pull request, #24991:
URL: https://github.com/apache/datafusion/pull/24991
## Which issue does this PR close?
- No issue filed. I hit this reading `get_index` in `regexpinstr.rs` to work
out how `N` and `subexpr` are meant to combine. Happy to file one if the
changelog needs the link.
## Rationale for this change
`regexp_instr` ignores `N` whenever `subexpr` is greater than zero. It
always reports a position inside the first match, whichever occurrence you
asked for.
```sql
> SELECT regexp_instr('12 abc def ghi 34 abc def ghi 56', '(abc) (def)
(ghi)', 1, 2, 'i', 2);
8 -- the 'def' in the first match. Should be 23, the 'def' in the second.
> SELECT regexp_instr('12 abc def ghi 34 abc def ghi 56', '(abc) (def)
(ghi)', 1, 3, 'i', 1);
4 -- there is no third match. Should be 0.
```
The second one is the worse shape: a query that asks for an occurrence which
does not exist gets a real position back, so nothing in the result says the
match was missing.
PostgreSQL, which this function follows, defines `regexp_instr` as returning
"the starting or ending position of the N'th match", and `subexpr` as "an
integer indicating which subexpression is of interest: the result identifies
the position of the substring matching that subexpression" of that match.
DataFusion's own doc string promises the two things independently: "N: Optional
The N-th occurrence of pattern to find" and "subexpr: Optional Specifies which
capture group (subexpression) to return the position for".
## What changes are included in this PR?
`get_index` in `datafusion/functions/src/regex/regexpinstr.rs` picked the
match with `Regex::captures`, which only ever returns the first match, and
dropped `n` on that branch. It now uses `Regex::captures_iter(...).nth(n - 1)`,
the capture-group counterpart of the `find_iter(...).nth(n - 1)` the `subexpr
== 0` branch already used, so both branches count occurrences the same way. The
comment claiming subexpr took precedence over N goes with it.
`N = 1` is unchanged, because `captures_iter(...).nth(0)` and
`captures(...)` are the same match. That is why no existing test or `.slt`
expectation moves.
## What is the testing strategy for this PR?
Every existing test of `subexpr`, in `regexpinstr.rs` and in
`regexp/regexp_instr.slt` alike, used `N = 1`, which is how this survived.
`test_case_sensitive_regexp_instr_scalar_subexp` gains `N = 2` for each of
the three capture groups, `N = 3` where no such match exists, and a `start =
18` row so `start` and `subexpr` are exercised together. The harness already
runs every row through `Utf8`, `LargeUtf8` and `Utf8View`.
`regexp/regexp_instr.slt` gains the two queries above.
Reverting only `regexpinstr.rs` to `main` and keeping the new tests, both
fail:
```
$ cargo test --profile ci -p datafusion-functions --lib regexp_instr
panicked at datafusion/functions/src/regex/regexpinstr.rs:740:21:
assertion `left == right` failed: regexp_instr scalar test failed
left: Some(4)
right: Some(19)
$ cargo test --profile ci -p datafusion-sqllogictest --test sqllogictests --
regexp/regexp_instr
1. query result mismatch:
[SQL] SELECT regexp_instr('12 abc def ghi 34 abc def ghi 56', '(abc) (def)
(ghi)', 1, 2, 'i', 2);
[Diff] (-expected|+actual)
- 23
+ 8
2. query result mismatch:
[SQL] SELECT regexp_instr('12 abc def ghi 34 abc def ghi 56', '(abc) (def)
(ghi)', 1, 3, 'i', 1);
[Diff] (-expected|+actual)
- 0
+ 4
```
With the fix, on aarch64-apple-darwin, rustc 1.97.0:
```
$ cargo test --profile ci -p datafusion-functions --lib
test result: ok. 341 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
$ cargo test --profile ci -p datafusion-sqllogictest --test sqllogictests --
regexp
Progress: 5/5 files completed (100%)
$ cargo fmt --all -- --check
$ cargo clippy --profile ci -p datafusion-functions --all-targets -- -D
warnings
```
## Are there any user-facing changes?
Yes. `regexp_instr(str, regexp, start, N, flags, subexpr)` with `N` greater
than 1 and `subexpr` greater than 0 now returns a position in the N'th match,
and 0 when there is no N'th match. No API change.
One thing I did not touch: `expr_fn::regexp_instr` in
`datafusion/functions/src/regex/mod.rs` takes an `endoption` argument and
pushes it as the fifth positional argument. PostgreSQL has that parameter, but
this UDF does not, so its fifth argument is `flags`. Any caller passing
`Some(endoption)` builds a seven-argument call that no signature accepts. That
looked like a separate change to me, and possibly an API break, so I left it
alone.
--
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]
