Implementation for regex_instr #15928

nirnayroy · 2025-05-02T20:02:33Z

Which issue does this PR close?

Closes Add regexp function - regexp_instr() #13009

Rationale for this change

Implements a regex SQL standard function in datafusion

What changes are included in this PR?

Implementation, tests, benches and docs for the regexp_instr function

Are these changes tested?

Yes

Are there any user-facing changes?

Yes

No

2010YOUY01 · 2025-05-04T03:01:13Z

Thank you. I'm wondering what's the reference system for this function's behavior (like postgres or others)

nirnayroy · 2025-05-04T06:56:41Z

Thank you. I'm wondering what's the reference system for this function's behavior (like postgres or others)

The reference system for this function's behaviour is postgres.

alamb · 2025-05-07T20:37:15Z

Thank you for this PR @nirnayroy

Can you please resolve the CI error: https://github.com/apache/datafusion/actions/runs/14820525339/job/41754009017?pr=15928

If you encounter an error, run './dev/update_function_docs.sh' and commit

nirnayroy · 2025-05-19T12:46:25Z

Thank you for this PR @nirnayroy

Can you please resolve the CI error: https://github.com/apache/datafusion/actions/runs/14820525339/job/41754009017?pr=15928

If you encounter an error, run './dev/update_function_docs.sh' and commit

I ran the bash script, but I’m not sure if the workflow succeeded.

nirnayroy · 2025-05-19T21:53:06Z

fixed the cippy errors showing up in the workflow

nirnayroy · 2025-05-20T05:40:21Z

fixed formatting error in workflow

alamb · 2025-05-21T18:46:37Z

@Omega359 I wonder if you might have time to review this PR?

Omega359 · 2025-05-21T19:35:08Z

Of course @alamb, not sure how I missed this one. It may be a day or two though

blaginin

Thank you for the PR!

I've left some comments - a few are just nits, so feel free to dismiss them or raise separate issues on top of this PR.

My main concern right now is that I’d expect this function to be a superset of strpos, but it currently behaves differently in some edge cases. For example:

SELECT regexp_instr('😀abcdef', 'abc');
SELECT strpos('😀abcdef', 'abc');

SELECT strpos(NULL, 'abc');
SELECT regexp_instr(NULL, 'abc');

SELECT strpos('abc', NULL);
SELECT regexp_instr('abc', NULL);

Do you think we should unify them? 🙏

datafusion/functions/src/regex/regexpinstr.rs

nirnayroy · 2025-06-12T07:10:36Z

Hi @blaginin, thanks for the review and regret the delay in reply. I think I have rectified a majority of the concerns raised. Please have a look again.

datafusion/functions/src/regex/regexpcount.rs

Copilot

Pull Request Overview

This PR adds the regexp_instr SQL function, including documentation, SQL logic tests, benchmarking, and integration into the function registry.

Introduce regexp_instr in docs and user guide
Add regexp_instr SQL tests and benches
Expose regex compilation helpers and register the new UDF

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
docs/source/user-guide/sql/scalar_functions.md	Add docs for `regexp_instr`, including signature and examples
datafusion/sqllogictest/test_files/regexp/regexp_instr.slt	Add SQL logic tests covering `regexp_instr` behavior
datafusion/functions/src/regex/regexpcount.rs	Make `compile_and_cache_regex` and `compile_regex` public
datafusion/functions/src/regex/mod.rs	Register `regexp_instr` UDF and define its expression builder
datafusion/functions/benches/regx.rs	Add benchmark cases for `regexp_instr`

Comments suppressed due to low confidence (4)

docs/source/user-guide/sql/scalar_functions.md:1844

The start argument description is duplicated (- **start**: - **start**:). Remove the extra label for clarity.

- **start**: - **start**: Optional start position (the first position is 1) to search for the regular expression. Can be a constant, column, or function. Defaults to 1

docs/source/user-guide/sql/scalar_functions.md:1837

The function signature is missing the subexpr (and return_option) parameters implemented in code. Update the signature to match the actual argument order.

regexp_instr(str, regexp[, start[, N[, flags]]])

datafusion/functions/src/regex/mod.rs:71

[nitpick] The parameter name endoption is unclear. Consider renaming it to return_option or another descriptive name consistent with SQL terminology.

        endoption: Option<Expr>,

datafusion/sqllogictest/test_files/regexp/regexp_instr.slt:1

There are no tests covering the subexpr argument. Add test cases that use the subexpr parameter to validate capture-group position behavior.

# Licensed to the Apache Software Foundation (ASF) under one

Copilot · 2025-06-17T20:36:53Z

datafusion/functions/benches/regx.rs

+                regexp_instr_func(&[
+                    Arc::clone(&data),
+                    Arc::clone(&regex),
+                    Arc::clone(&start),


The benchmark passes flags as the fourth argument, but according to the UDF signature it should be the fifth. Add the missing n (occurrence) argument or adjust the argument order.

Suggested change

Arc::clone(&start),

Arc::clone(&start),

Arc::new(n(&mut rng)) as ArrayRef,

datafusion/functions/src/regex/regexpinstr.rs

blaginin

Thanks for your work, @nirnayroy!

The PR is much better now, but I think there's still room for improvement:

Tests are failing. If that helps, you can run the CI command locally to debug:

The code around handling literals and arrays feels a bit complex. I think arrow/df does this in a simpler and more concise way – I’ve attached some links for inspiration :)

I haven’t reviewed the tests yet since I assume things might change as you iterate. But I can see you're covering a lot of cases, which is great ☘️

datafusion/functions/src/regex/regexpcount.rs

blaginin · 2025-06-18T22:59:16Z

datafusion/functions/src/regex/regexpinstr.rs

+enum ScalarOrArray<T> {
+    Scalar(T),
+    Array(Vec<T>),
+}
+
+impl<T: Clone> ScalarOrArray<T> {
+    fn iter(&self, len: usize) -> Box<dyn Iterator<Item = T> + '_> {
+        match self {
+            ScalarOrArray::Scalar(val) => Box::new(std::iter::repeat_n(val.clone(), len)),
+            ScalarOrArray::Array(arr) => Box::new(arr.iter().cloned()),
+        }
+    }


I don't think this construction is needed - it makes the code much more harder to read and no other regex function uses it. I think you may be reinventing Datum which is exactly "A possibly Scalar"

https://docs.rs/arrow/latest/arrow/array/trait.Datum.html

Also see ColumnarValue - this may also fit your needs

Example where that's used: datafusion/functions/src/regex/regexpmatch.rs

blaginin · 2025-06-18T23:03:02Z

datafusion/functions/src/regex/regexpinstr.rs

+    } else if len == 1 {
+        ScalarOrArray::Scalar(1)
+    } else {
+        ScalarOrArray::Array(vec![1; len])


I think this code may be too complex (and expensive). For example, if start_input was passed is None, you can pass none along later and handle that nullable case (same for other params). This will save you all this big processing "defaults" here

blaginin · 2025-06-18T23:05:07Z

datafusion/functions/src/regex/regexpinstr.rs

+    let flags_input = if let Some(ref flags) = flags_array {
+        if is_flags_scalar || flags.len() == 1 {
+            ScalarOrArray::Scalar(flags.value(0))
+        } else {
+            let flags_vec: Vec<&str> = flags.iter().map(|v| v.unwrap_or("")).collect();
+            ScalarOrArray::Array(flags_vec)


same as above, you don't need to set flags be "" - they can be null. Can you please check existing implementations for example? e.g. https://arrow.apache.org/rust/src/arrow_string/regexp.rs.html#356

blaginin · 2025-06-18T23:08:27Z

datafusion/functions/src/regex/regexpinstr.rs

+    .map(|(value, regex, start, nth, flags, subexp)| match regex {
+        None => Ok(None),
+        Some("") => Ok(None),
+        Some(regex) => get_index(


not sure that's wrong but not sure i get your idea - isn't regex a mandatory argument? how can it be none? and why is this allowed?

blaginin · 2025-06-18T23:10:40Z

datafusion/functions/src/regex/regexpinstr.rs

+        let match_start_byte_offset = byte_start_offset + mat.start();
+        let match_start_char_offset =
+            value[..match_start_byte_offset].chars().count() as i64 + 1;
+        Ok(Some(match_start_char_offset))


(nit), if you want, this can be each small own function

Implementation for regex_instr

45b45c3

github-actions bot added documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels May 2, 2025

linting and typo addressed in bench

4fb0cca

prettier formatting

cff8d42

scalar_functions_formatting

2e70881

alamb and others added 2 commits May 19, 2025 13:49

Merge branch 'main' into complete-regexp-instr

0b12a21

linting format macros

c3131ed

formatting

eeeac8d

Merge branch 'main' into complete-regexp-instr

4bd7eb6

blaginin self-requested a review May 21, 2025 16:54

blaginin reviewed May 21, 2025

View reviewed changes

Nirnay Roy added 3 commits June 12, 2025 02:23

address comments to PR

89e2315

formatting

fad6e7b

clippy

93c35c8

fmt

990a653

blaginin self-requested a review June 17, 2025 20:25

blaginin reviewed Jun 17, 2025

View reviewed changes

datafusion/functions/src/regex/regexpcount.rs Outdated Show resolved Hide resolved

blaginin requested a review from Copilot June 17, 2025 20:35

Copilot AI reviewed Jun 17, 2025

View reviewed changes

blaginin reviewed Jun 17, 2025

View reviewed changes

datafusion/functions/src/regex/regexpinstr.rs Outdated Show resolved Hide resolved

address docs typo

1120043

blaginin requested changes Jun 18, 2025

View reviewed changes

	Arc::clone(&start),
	Arc::clone(&start),
	Arc::new(n(&mut rng)) as ArrayRef,

Implementation for regex_instr #15928

Are you sure you want to change the base?

Implementation for regex_instr #15928

Conversation

nirnayroy commented May 2, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

2010YOUY01 commented May 4, 2025

Uh oh!

nirnayroy commented May 4, 2025

Uh oh!

alamb commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nirnayroy commented May 19, 2025

Uh oh!

nirnayroy commented May 19, 2025

Uh oh!

nirnayroy commented May 20, 2025

Uh oh!

alamb commented May 21, 2025

Uh oh!

Omega359 commented May 21, 2025

Uh oh!

blaginin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nirnayroy commented Jun 12, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blaginin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blaginin Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blaginin Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

blaginin Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

blaginin Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

blaginin Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

blaginin Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented May 7, 2025 •

edited

Loading

blaginin left a comment •

edited

Loading

blaginin Jun 18, 2025 •

edited

Loading