You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/09/25 07:59:29 UTC
(datafusion) 01/01: fix: apply regexp_instr subexpr to the N-th match, not the first (#24991)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-24991-5da95ac4726e415732e783532803a8e464f99dc8
in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit 0c7a085f6915bfa01470125cfd360ecfa6886123
Author: Max Freedom Pollard <27...@users.noreply.github.com>
AuthorDate: Fri Sep 25 07:59:05 2026 +0000
fix: apply regexp_instr subexpr to the N-th match, not the first (#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.
---------
Co-authored-by: Siew Kam Onn <ko...@gmail.com>
---
datafusion/functions/src/regex/regexpinstr.rs | 46 ++++++++++++++++------
.../test_files/regexp/regexp_instr.slt | 12 ++++++
2 files changed, 45 insertions(+), 13 deletions(-)
diff --git a/datafusion/functions/src/regex/regexpinstr.rs b/datafusion/functions/src/regex/regexpinstr.rs
index 5e1f5293b4..99990f0067 100644
--- a/datafusion/functions/src/regex/regexpinstr.rs
+++ b/datafusion/functions/src/regex/regexpinstr.rs
@@ -373,8 +373,9 @@ impl<'a> RegexCache<'a> {
/// Returns the 1-based character position of the `n`-th match of `pattern` in
/// `value`, or 0 if there is no such match. The search begins at the 1-based
-/// character position `start`. A positive `subexpr` selects that capture group
-/// of the first match instead of the `n`-th match.
+/// character position `start`. A positive `subexpr` returns the position of
+/// that capture group within the `n`-th match, or 0 if the match has no such
+/// group.
fn get_index(
value: &str,
pattern: &Regex,
@@ -395,17 +396,20 @@ fn get_index(
};
let search_slice = &value[byte_start_offset..];
- // A subexpression, when requested, takes precedence over the N-th match.
+ // `n` is 1-based, `nth` is 0-based.
+ let nth = (n - 1) as usize;
+
+ // A subexpression, when requested, is located within the N-th match.
let match_start = if subexpr > 0 {
pattern
- .captures(search_slice)
+ .captures_iter(search_slice)
+ .nth(nth)
.and_then(|captures| captures.get(subexpr as usize))
.map(|matched| matched.start())
} else {
- // `n` is 1-based, `nth` is 0-based.
pattern
.find_iter(search_slice)
- .nth((n - 1) as usize)
+ .nth(nth)
.map(|matched| matched.start())
};
@@ -690,13 +694,29 @@ mod tests {
}
fn test_case_sensitive_regexp_instr_scalar_subexp() {
- let values = ["12 abc def ghi 34"];
- let regex = ["(abc) (def) (ghi)"];
- let start = [1];
- let nth = [1];
- let flags = ["i"];
- let subexps = [2];
- let expected: Vec<i64> = vec![8];
+ // The first row locates a subexpression in the only match. The rest
+ // locate one in the N-th match, including an N that has no match.
+ let values = [
+ "12 abc def ghi 34",
+ "12 abc def ghi 34 abc def ghi 56",
+ "12 abc def ghi 34 abc def ghi 56",
+ "12 abc def ghi 34 abc def ghi 56",
+ "12 abc def ghi 34 abc def ghi 56",
+ "12 abc def ghi 34 abc def ghi 56",
+ ];
+ let regex = [
+ "(abc) (def) (ghi)",
+ "(abc) (def) (ghi)",
+ "(abc) (def) (ghi)",
+ "(abc) (def) (ghi)",
+ "(abc) (def) (ghi)",
+ "(abc) (def) (ghi)",
+ ];
+ let start = [1, 1, 1, 1, 1, 18];
+ let nth = [1, 2, 2, 2, 3, 1];
+ let flags = ["i", "i", "i", "i", "i", "i"];
+ let subexps = [2, 1, 2, 3, 1, 2];
+ let expected: Vec<i64> = vec![8, 19, 23, 27, 0, 23];
izip!(
values.iter(),
diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_instr.slt b/datafusion/sqllogictest/test_files/regexp/regexp_instr.slt
index 18a07afaf8..17f6470b32 100644
--- a/datafusion/sqllogictest/test_files/regexp/regexp_instr.slt
+++ b/datafusion/sqllogictest/test_files/regexp/regexp_instr.slt
@@ -73,6 +73,18 @@ SELECT
----
11
+# The subexpression is located inside the N-th match, not inside the first one.
+query I
+SELECT regexp_instr('12 abc def ghi 34 abc def ghi 56', '(abc) (def) (ghi)', 1, 2, 'i', 2);
+----
+23
+
+# There is no third match, so there is no position to report for any subexpression.
+query I
+SELECT regexp_instr('12 abc def ghi 34 abc def ghi 56', '(abc) (def) (ghi)', 1, 3, 'i', 1);
+----
+0
+
statement error DataFusion error: Execution error: regexp_instr\(\) requires start to be 1-based
SELECT regexp_instr('123123123123', '123', 0);
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org