[opt](function) speed up count_substrings with constant pattern#64121
Open
Mryange wants to merge 1 commit into
Open
[opt](function) speed up count_substrings with constant pattern#64121Mryange wants to merge 1 commit into
Mryange wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: no blocking findings in the actual GitHub PR diff.
Critical checkpoint conclusions:
- Goal and tests: The PR optimizes
count_substringsfor constant patterns and adds a benchmark that cross-checks the new searcher path against the old naive path and the function result. Existing regression coverage already includes constant-pattern and start-position cases. - Scope: The production code change is small and focused on the
pattern_constpath; benchmark additions are isolated underbe/benchmark. - Concurrency: No shared mutable state or thread-safety issue found in the changed function path.
- Lifecycle/static initialization: No new cross-TU static initialization dependency found; benchmark constants are local to the included benchmark translation unit.
- Configuration/compatibility/persistence: No config, storage-format, FE/BE protocol, or persistence changes in the actual PR diff.
- Parallel paths: Non-constant-pattern behavior is preserved; both 2-argument and 3-argument constant-pattern paths were updated consistently.
- Special conditions: Empty pattern and out-of-range start positions are handled consistently with the existing behavior.
- Test coverage: Existing regression tests cover
count_substrings; the new benchmark also validates result equivalence before timing. I did not run tests in this review environment. - Observability: Not applicable for this local scalar function optimization.
- Performance: The change reuses a prebuilt searcher for constant patterns and removes repeated byte-by-byte scans in that path; no obvious regression found during static review.
User focus: No additional user-provided review focus was specified.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28187 ms |
Contributor
TPC-DS: Total hot run time: 169030 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
What problem does this PR solve?
count_substrings(str, pattern)previously scanned each row byte by byte and compared the pattern at every candidate offset withmemcmp_small_allow_overflow15. This is expensive whenpatternis constant across a block, especially for long strings or rare matches.Root cause: the existing implementation did not reuse a prebuilt string searcher for constant patterns, so every row still used the naive per-offset comparison path.
This change builds one
ASCIICaseSensitiveStringSearcherper block whenpatternis constant and uses it to count non-overlapping matches. The non-constant pattern path is unchanged. A BE benchmark was added forcount_substrings(str, const_pattern)to compare the old naive path,StringSearch, direct searcher, and the actual function path.Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)