Repository navigation
perf: optimize char -> byte offset mapping in regexp_count - #24153
Merged
neilconway merged 1 commit intoAug 10, 2026
Merged
Conversation
Contributor
Author
|
FYI @iamhaseebn this extends the work in #24054; would love your review if you get the chance |
github-actions
Bot
added
sqllogictest
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24153 +/- ##
==========================================
- Coverage 81.05% 81.05% -0.01%
==========================================
Files 1106 1106
Lines 380556 380564 +8
Branches 380556 380564 +8
==========================================
+ Hits 308477 308479 +2
- Misses 53861 53863 +2
- Partials 18218 18222 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
iamhaseebn
approved these changes
Aug 7, 2026
iamhaseebn
left a comment
iamhaseebn
left a comment
Contributor
There was a problem hiding this comment.
Thanks, @neilconway! I reviewed the changes. The shared helper preserves the behavior from #24054, and the regexp_count update and tests cover the relevant cases. Overall, it looks good to me.
Contributor
Author
|
@iamhaseebn Awesome, thanks for the review! |
Rich-T-kid
approved these changes
Aug 8, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
@neilconway this looks good to me! thanks
Comment on lines
-568
to
-588
| let char_len = value.chars().count(); | ||
| let start_index = (start as usize).saturating_sub(1); | ||
|
|
||
| if start_index > char_len { | ||
| let Some(byte_offset) = start_to_byte_offset(value, start) else { | ||
| return Ok(0); | ||
| } | ||
|
|
||
| // Find the byte offset for the start position (1-based character index) | ||
| let byte_offset = if start_index == char_len { | ||
| value.len() | ||
| } else { | ||
| value | ||
| .char_indices() | ||
| .nth(start_index) | ||
| .map(|(idx, _)| idx) | ||
| .unwrap_or(value.len()) | ||
| }; | ||
|
|
||
| // Use string slicing instead of collecting chars into a new String | ||
| let find_slice = &value[byte_offset..]; | ||
| let count = pattern.find_iter(find_slice).count(); |
alamb
approved these changes
Aug 10, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thanks @iamhaseebn @neilconway and @Rich-T-kid
kosiew
pushed a commit
to kosiew/datafusion
that referenced
this pull request
Aug 12, 2026
…24153) ## Which issue does this PR close? - N/A ## Rationale for this change `regexp_count` takes an optional argument, `start`, that specifies a character-based position. The current implementation involves calling `start.chars().count` for every input row, which scans the entire string. This is unnecessarily expensive for the common case that `start` identifies a character position near the start of the string: once we've converted `start` into a byte offset, we can stop scanning the rest of the string. Fix this by doing a lazy walk of the string and stopping early. This was first done for `regexp_instr` in apache#24054; this PR refactors that code into a shared helper. In passing, this also fixes a 32-bit portability bug with the previous code in `regexp_count` as well. regexp_count benchmark, with_start cases: - `size=1024 str_len=32`: -5.7% - `size=1024 str_len=128`: -11.5% - `size=4096 str_len=32`: -4.1% - `size=4096 str_len=128`: -6.6% ## What changes are included in this PR? * Refactor char position -> byte offset mapping from `regexp_instr` into a new helper, `start_to_byte_offset` * Use `start_to_byte_offset` in both `regexp_instr` (no functional change) and `regexp_count` * Add SLT cases verifying that `regexp_count` handles zero-width patterns the same way that PostgreSQL does (this was the primary motivation for apache#24054) ## Are these changes tested? Yes — new unit tests for the shared helper; new SLT cases pin the existing zero-width-pattern behavior of the refactored path (verified against PostgreSQL 18.4); existing `regexp_instr`/`regexp_count` tests cover the rest. ## Are there any user-facing changes? No.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
regexp_counttakes an optional argument,start, that specifies a character-based position. The current implementation involves callingstart.chars().countfor every input row, which scans the entire string. This is unnecessarily expensive for the common case thatstartidentifies a character position near the start of the string: once we've convertedstartinto a byte offset, we can stop scanning the rest of the string.Fix this by doing a lazy walk of the string and stopping early. This was first done for
regexp_instrin #24054; this PR refactors that code into a shared helper. In passing, this also fixes a 32-bit portability bug with the previous code inregexp_countas well.regexp_count benchmark, with_start cases:
size=1024 str_len=32: -5.7%size=1024 str_len=128: -11.5%size=4096 str_len=32: -4.1%size=4096 str_len=128: -6.6%What changes are included in this PR?
regexp_instrinto a new helper,start_to_byte_offsetstart_to_byte_offsetin bothregexp_instr(no functional change) andregexp_countregexp_counthandles zero-width patterns the same way that PostgreSQL does (this was the primary motivation for fix: handle empty patterns in regexp_instr #24054)Are these changes tested?
Yes — new unit tests for the shared helper; new SLT cases pin the existing zero-width-pattern behavior of the refactored path (verified against PostgreSQL 18.4); existing
regexp_instr/regexp_counttests cover the rest.Are there any user-facing changes?
No.