Consolidate special case regexp_match logic#21486
Open
alamb wants to merge 1 commit intoapache:mainfrom
Open
Conversation
github-actions
bot
added
sqllogictest
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
run clickbench_partitioned |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
alamb
force-pushed
the
alamb/test_coverage
branch
from
2117a33 to
75d8e4c
Compare
alamb
commented
Apr 9, 2026
| let mut new_offsets = BufferBuilder::<T>::new(string_array.len() + 1); | ||
| new_offsets.append(T::zero()); | ||
|
|
||
| if let Some(ref short_re) = short_re { |
Contributor
Author
There was a problem hiding this comment.
The idea was to encapsulate this logic so it was easer to understand and not repeated in the two loops
| /// up the pre-processing time of the replacement string, since it only | ||
| /// needs to processed once. | ||
| fn _regexp_replace_static_pattern_replace<T: OffsetSizeTrait>( | ||
| fn regexp_replace_static_pattern_replace<T: OffsetSizeTrait>( |
Contributor
Author
There was a problem hiding this comment.
As a drive by fix, this function randomly was named starting with _ which is unecessary and unconventional
| .into_owned() | ||
| } | ||
|
|
||
| struct ShortRegex { |
Contributor
Author
There was a problem hiding this comment.
The idea is to consolidate the logic for the special case short regexp optimization so
- It is easier to understand
- It is not duplicated (and thus improve test coverage)
Contributor
Author
|
run benchmark regx |
Contributor
Author
|
run benchmark clickbench_partitioned |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing alamb/test_coverage (75d8e4c) to 8cf70ec (merge-base) diff File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing alamb/test_coverage (75d8e4c) to 8cf70ec (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
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
2 participants
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.
regexp_replaceby stripping trailing .* from anchored patterns. 2.4x improvement (ClickBench Q28) #21379Which issue does this PR close?
regexp_replaceby stripping trailing .* from anchored patterns. 2.4x improvement (ClickBench Q28) #21379 from @DandandanRationale for this change
#21379 adds a specific optimization, but it had some non trivial code duplication
I wanted to reduce the duplication and also make the code easier to read(in my opinion)
What changes are included in this PR?
Move the special RegExp logic into its own struct, and add copious comments
Are these changes tested?
By existing tests, and the additional tests added in
Are there any user-facing changes?
No this is internal changes only
I ran benchmarks and see no change in performance (as expected)