Skip to content

Add one-byte fast path for replaceAll string replacement#108178

Merged
cv4g merged 4 commits into
ClickHouse:masterfrom
groeneai:groeneai/replace-onebyte-fastpath-106598
Jul 2, 2026
Merged

Add one-byte fast path for replaceAll string replacement#108178
cv4g merged 4 commits into
ClickHouse:masterfrom
groeneai:groeneai/replace-onebyte-fastpath-106598

Conversation

@groeneai

@groeneai groeneai commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Related: #106598

Changelog category (leave one):

  • Performance Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Improve the performance of replaceAll and replaceRegexpAll when the pattern is a single character and the replacement is a single character (for example replaceRegexpAll(s, ' ', '_')). Such replacements no longer change the string layout, so the column is now copied once and matching bytes are rewritten in place instead of running a per-match search loop.

Description

replaceRegexpAll / replaceAll with a trivial single-character pattern fall back to ReplaceStringImpl::vectorConstantConstant. When both the needle and the replacement are a single byte in replace-all mode, every match keeps the string layout: offsets are unchanged. The general code path still runs a full Volnitsky search over the whole buffer with a resize + memcpy and offset bookkeeping per match.

This PR adds a fast path for that case: copy the ColumnString data and offsets once, then flip matching bytes in place. The needle is restricted to non-NUL so ColumnString row terminators are preserved. The output is byte-identical to the general path.

This addresses the ARM replaceRegexp_fallback query 0 regression in #106598. The reported regression came from a StringZilla ARM-codegen change that slowed the general search path; this fix is algorithmic and skips that path entirely for the single-byte case, so it helps on all architectures.

Local validation (x86-64 debug build) of replaceRegexpAll(materialize(s), ' ', '\n') over numbers_mt(5000000):

  • without fix: ~0.66s median (5 runs)
  • with fix: ~0.083s median (5 runs)

Correctness: a new test 03900_replace_one_byte_fast_path cross-checks the fast path against an independent splitByChar/arrayStringConcat oracle and covers no-match, all-match, adjacent-match, empty-string, First-mode (replaceRegexpOne), multi-byte replacement and multi-byte needle (both must use the general path), and FixedString inputs. An ad-hoc sweep over 200k random strings showed zero mismatches for both replaceRegexpAll and replaceAll. Existing replace/regexp tests still pass.

CI report from the regression: https://performance.ci.clickhouse.com/history/replaceRegexp_fallback/0?metric=client_time&arch=arm

Note on final ARM numbers: the controlled ARM Neoverse-V2 pipeline is not available on this build host, so the percentage recovery on ARM should be confirmed by the perf-CI / the reporter's pipeline.

Version info

  • Merged into: 26.7.1.429 (included in 26.7 and later)

replaceRegexpAll/replaceAll with a trivial single-character pattern fall
back to ReplaceStringImpl::vectorConstantConstant. For a one-byte needle
and one-byte replacement in replace-all mode the output layout is identical
to the input (offsets unchanged), so the per-match Volnitsky search loop is
unnecessary. Copy the ColumnString buffer once and flip matching bytes in
place instead.

The needle is restricted to non-NUL so ColumnString row terminators are
preserved. This recovers the replaceRegexp_fallback/0 performance test
(issue ClickHouse#106598): the slow path runs a full search plus repeated resize and
memcpy per row, while the fast path is a single bulk copy and a linear byte
scan.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

Pre-PR validation gate

# Question Answer
a Deterministic repro? Yes. replaceRegexpAll(materialize('Many years later ... ice.'), ' ', '\n') FROM numbers_mt(5000000) FORMAT Null deterministically hits the slow fallback path on every run. With the fix it deterministically takes the new fast path.
b Root cause explained? A single-char trivial pattern makes ReplaceRegexpImpl::vectorConstantConstant fall back to ReplaceStringImpl::vectorConstantConstant. For a one-byte needle + one-byte replacement in replace-all mode the output layout is identical to the input (offsets unchanged), yet the general code still runs a full Volnitsky search across the whole buffer with a resize+memcpy and offset bookkeeping per match. The reported ARM regression (#106598) came from a StringZilla ARM-codegen change that made that general search slower; the work itself was always avoidable.
c Fix matches root cause? Yes. For exactly the one-byte/one-byte/replace-all case, copy the ColumnString buffer + offsets once and flip matching bytes in place, skipping the search loop entirely. This is algorithmic, so it helps on all architectures rather than chasing the StringZilla ARM codegen.
d Test intent preserved / new tests added? New test 03900_replace_one_byte_fast_path added. It cross-checks the fast path against an independent splitByChar/arrayStringConcat oracle and covers no-match, all-needle, adjacent matches, empty string, First-mode (replaceRegexpOne must NOT take the all-mode fast path), one-byte-needle/multi-byte-replacement (general path), multi-byte trivial needle (general path), and FixedString inputs. No existing test was weakened.
e Both directions demonstrated? Yes. x86-64 debug, query 0 over numbers_mt(5000000), 5 runs each: without fix ~0.66s median, with fix ~0.083s median (~8x). Correctness: an ad-hoc sweep over 200k random strings produced zero mismatches vs the oracle for both replaceRegexpAll and replaceAll; the new test and existing replace/regexp tests pass via clickhouse-test. ARM percentage recovery should be confirmed on the perf-CI / reporter pipeline (no ARM hardware on this build host).
f Fix is general, not a narrow patch? Yes. The fast path lives in the shared ReplaceStringImpl::vectorConstantConstant used by both replaceAll (literal) and the replaceRegexpAll trivial-pattern fallback, so both benefit. It is correctly gated to replace-all mode only (First mode needs to stop after one match) and to a non-NUL one-byte needle so ColumnString row terminators are never altered. Other arities (vector needle/replacement) keep their existing behavior; no symptom-guarding involved.

Session id: cron:clickhouse-worker-slot-5:20260622-163100

@groeneai

Copy link
Copy Markdown
Contributor Author

cc @rschu1ze - could you review this? It adds a one-byte fast path to ReplaceStringImpl::vectorConstantConstant: when the needle and replacement are both a single non-NUL byte in replace-all mode, the output offsets are unchanged, so it copies the column once and rewrites matching bytes in place instead of running the per-match Volnitsky search. This recovers the ARM replaceRegexp_fallback/0 perf regression (#106598) and helps on all architectures (~8x faster on a local x86-64 debug run).

@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Jun 22, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [b9edb14]

Summary:

job_name test_name status info comment
AST fuzzer (amd_debug, targeted, old_compatibility) FAIL
Logical error: 'current_version != existing_version' (STID: 2508-3ef8) FAIL cidb, issue ISSUE EXISTS

AI Review

Summary

This PR adds fast paths for constant string replacement: needle == replacement now copies String/FixedString input directly, and one-byte replaceAll rewrites matching bytes in place when output layout is unchanged. The current code matches that contract, the previous NUL-byte concern is fixed, and the new stateless and performance tests cover the important paths. I found no blocking or major code issues.

Findings
  • 💡 Nits
    • [PR description] The description still says the needle is restricted to non-NUL so ColumnString row terminators are preserved. Current ColumnString rows are offset-delimited, the code now handles NUL needles, and the tests cover that path. Please remove or update that stale sentence so the PR record matches the implementation.
Missing context / blind spots
  • ⚠️ I could not run local tests because this checkout has no build directory or clickhouse binary. The latest Praktika PR report for b9edb14914acde33c410b1d79482ac8ea97a59de shows no failed checks, and the PR includes focused stateless plus performance coverage.
Final Verdict

Status: ✅ Approve

No required code changes. The PR-description nit can be cleaned up before merge.

@clickhouse-gh clickhouse-gh Bot added the pr-performance Pull request with some performance improvements label Jun 22, 2026
Comment thread src/Functions/ReplaceStringImpl.h Outdated
@clickhouse-gh

clickhouse-gh Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

📊 Cloud Performance Report

✅ AI verdict: no_change — no significant changes across 38 queries analysed

no significant changes detected. K_source=6 K_base=30 flagged=0/65

clickbench

🟢 No significant changes

tpch_adapted_1_official

🟢 No significant changes

Debug info
  • StressHouse run: efe4fa10-2259-48a2-a619-1ba9eaac1882
  • MIRAI run: 9827475d-f68b-4510-bfac-226504752ee2
  • PR check IDs:
    • clickbench_536932_1782851338
    • clickbench_536939_1782851338
    • clickbench_536949_1782851338
    • tpch_adapted_1_official_536957_1782851338
    • tpch_adapted_1_official_536970_1782851338
    • tpch_adapted_1_official_536980_1782851338

ColumnString rows are not NUL-terminated; row boundaries are defined by
the offsets array, not by an in-band terminator (see ColumnString.h: the
chars buffer may contain zero bytes anywhere in a row). The fast path
copies data and offsets verbatim and only flips matching data bytes
1-for-1, so it preserves row boundaries for any needle byte, including
'\0'. The previous needle[0] != 0 guard was therefore over-conservative
and made the changelog claim ("one-byte patterns") inaccurate.

Drop the guard and extend the test with NUL-needle and NUL-in-payload
cases, cross-checked byte-for-byte against an independent concat oracle.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

Pre-PR validation gate (follow-up: drop needle[0] != 0 guard, NUL needle in fast path)

# Question Answer
a Deterministic repro? N/A (not a bug fix). The "before" state was an over-conservative guard sending NUL needles down the general path. Behavior verified on demand: replaceAll(materialize(concat('a',char(0),'b',char(0),'c')), char(0), '_') returns a_b_c (hex 615F625F63).
b Root cause explained? src/Columns/ColumnString.h line 40: "strings are not zero-terminated and could contain zero bytes in the middle." Row boundaries come from the offsets array (sizeAt(i) = offsets[i] - offsets[i-1], no -1); there is no in-band terminator. The fast path copies data and offsets verbatim and only flips matching data bytes 1-for-1, so boundaries are preserved for any needle byte. The needle[0] != 0 guard was based on an obsolete NUL-terminated layout.
c Fix matches root cause? Yes. Dropped the guard so NUL needles take the fast path, and reworded the comment to state the actual invariant (offset-defined boundaries). This makes the code match the changelog claim ("one-byte patterns").
d Test intent preserved / new tests added? Yes. Existing assertions unchanged. Added NUL-needle and NUL-in-payload cases to 03900_replace_one_byte_fast_path, cross-checked byte-for-byte against an independent concat oracle (leading/trailing NUL, embedded NUL preserved while replacing a different byte, 1000-row cross-checks).
e Both directions demonstrated? Fast path output verified byte-identical (hex-level) to an independent concat oracle across 25k+ rows: NUL needle replace, NUL-in-payload preservation, leading/trailing NUL row boundaries, and row-length preservation (offsets untouched, 0 length changes over 5k rows). Negative cases confirmed: replaceOne (First mode) only replaces the first NUL; 1-byte needle + 2-byte replacement still uses the general path and is correct. 30/30 randomized runs pass.
f Fix is general, not a narrow patch? Yes. The 1-byte fast path lives only in ReplaceStringImpl::vectorConstantConstant; reached by replaceAll and replaceRegexpAll (All mode) and excluded from replaceOne/replaceRegexpOne via if constexpr (replace == All). No sibling implementation carries the same guard, so nothing else needs the change.

Session id: cron:clickhouse-worker-slot-2:20260622-212900

@groeneai

Copy link
Copy Markdown
Contributor Author

CI finished on cbcea65. The three reds are pre-existing trunk flakes, not caused by this PR (which only adds a one-byte fast path in ReplaceStringImpl::vectorConstantConstant; none of the failing queries involve string replacement):

  • BuzzHouse (amd_debug) / Inconsistent AST formatting (ExpressionList), STID 1941-1bfa: a fuzzed EXPLAIN PLAN over a JSON literal. Chronic AST-formatting roundtrip family (40+ PRs in 90d), unrelated to replaceAll.
  • Stress test (arm_tsan) / Not-ready Set, STID 0250-41a5: globalNotIn Not-ready Set abort. Chronic prepared-sets family (40 PRs in 60d).
  • Integration tests (amd_llvm_coverage, 4/5) / Timeout: generic session-timeout occurred during test execution, infra.

Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=108178&sha=cbcea65aa4e1d198c6d2332f350e9295bb652ad9&name_0=PR&name_1=Stress%20test%20%28arm_tsan%29

The one-byte fast-path test (incl. the NUL-needle / NUL-in-payload oracle cross-checks) passes.

@groeneai

Copy link
Copy Markdown
Contributor Author

CI status: the red checks here are pre-existing chronic trunk flakies / infra, not caused by this change (which only touches ReplaceStringImpl.h).

  • BuzzHouse (amd_debug) "Inconsistent AST formatting (ExpressionList)" (STID 1941-1bfa): chronic trunk fuzzer bug, 75 unrelated PRs + 10 master hits in 30d.
  • Stress test (arm_tsan) "Not-ready Set is passed as the second argument for function globalNotIn" (STID 0250-41a5): chronic trunk flake, 32 unrelated PRs + 15 master hits in 30d.
  • Integration tests (amd_llvm_coverage, 4/5) session Timeout: coverage-batch infra timeout, not a test failure.

Functional test checks are green and the new 03900_replace_one_byte_fast_path test passes. None of the reds touch the string-replace path.

@cv4g cv4g self-assigned this Jun 30, 2026
@cv4g

cv4g commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

@groeneai Great! Should we also add an early exit on needle == replacement?

@@ -0,0 +1,76 @@
-- Validates the one-byte-needle/one-byte-replacement "replace all" fast path in ReplaceStringImpl

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@groeneai Performance optimizations also need a performance test (tests/performance/*.xml). Please write such a test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests/performance/replace_string_fast_path.xml in b9edb14. It exercises both fast paths over a materialized 5M-row column: the one-byte needle/replacement byte-flip (replaceAll(s, ' ', '_')) and the needle == replacement no-op for one-byte, multi-byte, replaceOne, and FixedString. Modeled on the existing replaceRegexp_fallback.xml; the controlled ARM Neoverse numbers come from perf-CI, locally I confirmed the queries are well-formed and hit the new branches.

Replacing the needle with itself produces output identical to the input
for any needle length and both replace modes, since each match copies the
replacement (equal to the matched bytes) verbatim and non-matching bytes
are passed through unchanged. Detect this up front and copy the column
buffer and offsets once instead of running the Volnitsky search.

This subsumes and generalizes the one-byte fast path for the
needle == replacement case (it also covers multi-byte needles and First
mode). The same shortcut is added to the FixedString overload, where the
offsets are rebuilt from the fixed row width.

Extend 03900 with needle == replacement cases (one-byte, multi-byte,
First mode, FixedString, NUL) cross-checked against the unmodified input.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

Done in 455089a. Added it as a top-level needle == replacement early exit rather than scoping it to the one-byte case: replacing the needle with itself is a no-op for any needle length and both replace modes, so it copies the column buffer and offsets once and skips the Volnitsky search entirely (also subsumes the one-byte path for this case). Same shortcut added to the FixedString overload (offsets rebuilt from the fixed width). Extended 03900 with one-byte, multi-byte, First-mode, FixedString and NUL cases, cross-checked byte-for-byte against the unmodified input.

Covers the one-byte needle/replacement byte-flip path and the
needle == replacement no-op path in ReplaceStringImpl, addressing
the request for a perf test in PR ClickHouse#108178.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

Perf test added in b9edb14 (tests/performance/replace_string_fast_path.xml).

Pre-PR validation gate (click to expand)
# Question Answer
a Deterministic repro? N/A — perf-test addition, not a bug fix. The optimized paths were correctness-validated earlier (commits 0cb65bd, cbcea65, 455089a).
b Root cause explained? N/A — no bug. Test exercises the fast paths added for #106598 (ARM replaceRegexp_fallback regression).
c Fix matches root cause? N/A — no code change in this commit.
d Test intent preserved / new tests added? Yes — adds a perf test exercising the one-byte byte-flip path and the needle == replacement no-op. Functional correctness stays covered by 03900_replace_one_byte_fast_path.
e Both directions demonstrated? N/A (perf). Locally confirmed all 5 queries are well-formed, parse, and execute, hitting the new branches; controlled ARM numbers come from perf-CI.
f General across code paths? Yes — covers both vectorConstantConstant (String) and vectorFixedConstantConstant (FixedString).
g Generalizes across inputs? Yes — one-byte, multi-byte, replaceAll vs replaceOne (All/First), String + FixedString.
h Backward compatible? N/A — test-only, no behavior/format/setting change.
i Invariants and contracts preserved? N/A — declarative perf test only, no code change.

Session id: cron:clickhouse-worker-slot-5:20260630-164800

@clickhouse-gh

clickhouse-gh Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 85.40% 85.40% +0.00%
Functions 92.70% 92.70% +0.00%
Branches 77.70% 77.70% +0.00%

Changed lines: Changed C/C++ lines covered: 19/20 (95.00%) · Uncovered code

Full report · Diff report

@groeneai

Copy link
Copy Markdown
Contributor Author

CI finish ledger — b9edb14

Every failure below has an owner. Only CH Inc sync is exempt.

Check / test Reason Owner / fixing PR
AST fuzzer (amd_debug, targeted, old_compatibility) / LOGICAL_ERROR current_version != existing_version (STID 2508-3ef8) chronic trunk bug in PatchJoinCache::Entry::addBlock (3 unrelated PRs, 1 master / 30d) — not PR-caused (this PR adds a one-byte replaceAll fast path; touches no patch-join / lightweight-update code) a full-effort fix task is investigating (fixing-PR link to follow here)
CH Inc sync CH Inc sync (private, not actionable)

All other AST fuzzer jobs were Fuzzer exited with timeout = success. No PR-caused failure.

Session id: cron:our-pr-ci-monitor:20260630-213000

@cv4g
cv4g added this pull request to the merge queue Jul 2, 2026
Merged via the queue into ClickHouse:master with commit 7408f67 Jul 2, 2026
172 of 174 checks passed
@rschu1ze

rschu1ze commented Jul 2, 2026

Copy link
Copy Markdown
Member
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-performance Pull request with some performance improvements pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants