AnthonyZhOon opened a new pull request, #26021:
URL: https://github.com/apache/datafusion/pull/26021
## Which issue does this PR close?
- Closes #23675.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in
the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your
changes and offer better suggestions for fixes.
Please explain the problem you are trying to solve in terms of the
user-visible
behavior, rather than the implementation.
For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the
implementation. "COUNT(DISTINCT) returns wrong results when the column
contains
nulls" is the user-visible problem.
-->
As stated in the issue, there are two implementations of regex caching and
they are not utilised across all the `regexp*` functions. This PR aims to
improve the performance of `regexp*` functions by caching a compiled regex to
be reused.
## What changes are included in this PR?
Disclaimer: The code was written by GPT 6.1 Sol but went through multiple
revisions and I understand the changes made, performance choices in the design
were backed by benchmark improvements (cold, inline, cache shape)
This change unifies the regex caching approach for the `regex` module and
uses it for the `regexp_{count,instr,replace}` functions. The `regexp_match`
and `regexp_like` functions are not changed as I noticed they deferred array
processing to arrow which already used a single compiled regex in a batch and
are reasonably fast.
> Though rewriting the inner loop to use the RegexCache still had
performance improvements but I was worried it would affect correctness with me
being unfamiliar with how Arrow handles regex on arrays.
## What is the testing strategy for this PR?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
Briefly describe how this PR is tested, and point to the specific tests you
added. For example: 'This new feature is covered by the `sqllogictest` cases
added in `foo.slt`'.
If this PR does not add tests, explain why. For example, if the change is
already covered by existing tests, please mention it.
You should also check the `codecov` bot reply on this PR to confirm the
changed code is exercised.
-->
This PR adds unit tests for RegexCache behaviour (caching means compiling
only once and cache key makes sense) and criterion benchmarks for the `regexp*`
functions that use the cache.
## Are there any user-facing changes?
Just performance improvements in general though 1 case regresses
Below is the criterion benchmark result.
Mode | Pattern sequence | What it measures
-- | -- | --
Repeated | (a+), (a+), (a+), … | One compilation, followed by consecutive
reuse through the cache’s previous-pattern shortcut.
Alternating | (a+), (b+), (a+), (b+), … | Two compilations, then reuse
through hash lookups because the pattern changes every row.
Distinct | (a+)(?:){0}, (a+)(?:){1}, (a+)(?:){2}, … | A unique pattern
string per row, forcing 1,024 compilations and cache insertions.
| Function / patterns | Utf8: baseline → patch | Utf8View: baseline → patch |
|---|---:|---:|
| count / repeated | 399.7 MB/s → **539.8 MB/s** 🟢 +35.1% | 366.4 MB/s →
**519.5 MB/s** 🟢 +41.8% |
| count / alternating | 354.0 MB/s → **392.9 MB/s** 🟢 +11.0% | 323.3 MB/s →
**386.1 MB/s** 🟢 +19.4% |
| count / distinct | 3.05 MB/s → **3.17 MB/s** 🟢 +4.1%† | 2.88 MB/s → **3.13
MB/s** 🟢 +8.6% |
| instr / repeated | 443.5 MB/s → **475.4 MB/s** 🟢 +7.2% | 453.7 MB/s →
**473.3 MB/s** 🟢 +4.3%† |
| instr / alternating | 319.9 MB/s → **348.3 MB/s** 🟢 +8.9% | 330.9 MB/s →
**344.1 MB/s** 🟢 +4.0% |
| instr / distinct | 2.85 MB/s → **2.94 MB/s** 🟢 +3.3% | 2.82 MB/s → **2.89
MB/s** 🟢 +2.3% |
| replace / repeated† | 161.5 MB/s → **168.7 MB/s** 🟢 +4.5%† | 172.3 MB/s →
**170.4 MB/s** 🔴 -1.1%† |
| replace / alternating† | 177.1 MB/s → **166.8 MB/s** 🔴 -5.8%† | 177.2 MB/s
→ **163.5 MB/s** 🔴 -7.8%† |
| replace / distinct | 2.66 MB/s → **2.84 MB/s** 🟢 +7.0% | 2.70 MB/s →
**2.81 MB/s** 🟢 +4.2% |
| replace / repeated, flags | 119.4 MB/s → **160.3 MB/s** 🟢 +34.2% | 121.9
MB/s → **167.4 MB/s** 🟢 +37.3% |
| replace / alternating, flags | 117.8 MB/s → **141.2 MB/s** 🟢 +19.8% |
116.9 MB/s → **130.0 MB/s** 🟢 +11.2%† |
| replace / distinct, flags | 2.54 MB/s → **2.60 MB/s** 🟢 +2.6% | 2.57 MB/s
→ **2.68 MB/s** 🟢 +4.1% |
†: replace already had a cache implementation of `HashMap<String, Regex>` so
the penalty is in the new most recently used `last` lookup that we add to the
cache. Which always misses in an alternating worst case causing an extra check
in this cache miss path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
