fix: handle empty patterns in regexp_instr - #24054
Conversation
neilconway
left a comment
There was a problem hiding this comment.
Thanks @iamhaseebn ! Overall this looks good: the behavior change is in the right direction, and the implementation is sound.
It seems like this PR also changes behavior for two other cases, not mentioned in the PR description:
- Patterns that might be empty width, like
regexp_instr('abc', 'x*', 4) - Empty patterns on NULL input, like
regexp_instr(NULL, '')
Both changes seem to make our behavior more consistent with PG, so I think they are good, but can you update the PR description and ensure there is test coverage for these cases?
| @@ -479,7 +482,7 @@ mod tests { | |||
| fn test_case_sensitive_regexp_instr_nulls() { | |||
There was a problem hiding this comment.
Not yours, but this function seems misnamed (doesn't involve NULLs). This seems redundant with other tests anyway, so we can probably just remove this function.
|
Thanks for calling these out. I updated the PR description and added regression coverage for both cases:
The focused unit test, SQL logic test, formatting check, and targeted Clippy check pass. The update is in 71fbe61. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24054 +/- ##
==========================================
+ Coverage 80.86% 80.87% +0.01%
==========================================
Files 1101 1101
Lines 375446 375816 +370
Branches 375446 375816 +370
==========================================
+ Hits 303592 303954 +362
+ Misses 53761 53754 -7
- Partials 18093 18108 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Which issue does this PR close?
regexp_instrwith an empty pattern should return 1 #22257.Rationale for this change
PostgreSQL treats empty and other zero-width regular-expression matches as valid character-boundary matches. Before this change,
regexp_instrreturned 0 before compiling an empty pattern, excluded the terminal boundary from its start-position mapping, and returned 0 instead of propagating NULL when the input was NULL and the pattern was empty.What changes are included in this PR?
Are these changes tested?
Yes. The following checks pass on the current head:
cargo fmt --all -- --checkcargo test -p datafusion-functions regexpinstr::tests::test_regexp_instrcargo test --profile=ci --test sqllogictests -- regexp_instr.sltcargo clippy -p datafusion-functions --tests -- -D warningsThe implementation commit was also verified with the repository-prescribed extended workspace suite and
cargo test --profile ci -p datafusion-cli; the review follow-up changes add tests only.Are there any user-facing changes?
Yes.
regexp_instrnow returns PostgreSQL-compatible positions for empty and potentially zero-width patterns, including at the terminal character boundary, and returns NULL for NULL input with an empty pattern. This does not change the public API.