Pre-PR validation gate

Session id: cron:clickhouse-worker-slot-8:20260622-163200

cc @rschu1ze — could you review this? It adds a fast path to default trimLeft/ltrim: when no row in the column starts with a space (checked by a cheap per-row first-byte scan), the input column is returned directly instead of running the per-row symbol scan and copying the whole column. This recovers the ~12-14% ARM regression in trim_numbers query 1 reported in #106601 (only the default-whitespace left-trim path is touched; trimRight/trimBoth/custom-chars/FixedString are unchanged).

Workflow [PR], commit [c8966fe]

Summary: ❌

AI Review

Summary

This PR rewrites the default-space trimLeft/trimRight/trimBoth string path to batch rows that need no trimming, then uses the existing SIMD space scan only for rows that actually start or end with a space. The current code shape looks consistent with the latest maintainer feedback, but I would not approve it yet because the PR metadata still describes an older direct-return implementation and the latest commit's performance CI is still pending.

PR Metadata

• Changelog category: Performance Improvement is appropriate.

• Changelog entry: required and present, but currently inaccurate. It says the input column is "returned directly"; the current implementation always returns col_res and copies unchanged rows in a batch. Suggested replacement:

  Speed up default-whitespace trim functions, including trimLeft/ltrim, by batching unchanged rows into one copy and only running the space scan for rows with leading or trailing spaces.

• The description should also be updated: the current implementation is no longer limited to only default-whitespace trimLeft/ltrim, and it does not return the original ColumnString. This is the same unresolved metadata issue as the existing bot thread: Speed up default trimLeft when no row needs trimming #108177 (comment).

• The Closes: https://github.com/ClickHouse/ClickHouse/issues/106601 line is still inside the template HTML comment, so it will not be rendered as an active PR relationship. Move it outside the comment block if this PR should close that issue.

Missing context / blind spots

• ⚠️ Latest Praktika for c8966fe52bc3ca169cee306629a05478aaa06057 is still running: builds, stateless tests, and both AMD/ARM Performance Comparison shards were pending when reviewed. Completed CI would close this gap.
• ⚠️ The PR is a Performance Improvement, but machine-readable ARM performance results for the latest commit are not available yet. The key evidence is trim_numbers improvement without reintroducing the trim_whitespace regression.

Tests

• ⚠️ Wait for the latest ARM Performance Comparison, or provide equivalent controlled Neoverse-V2 numbers, showing the current commit keeps the intended trim_numbers speedup and restores trim_whitespace to master parity.

Final Verdict

Status: ⚠️ Request changes

Minimum required actions: update the PR body/changelog to match the final implementation and wait for current CI/performance evidence on c8966fe52bc3ca169cee306629a05478aaa06057.

📊 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

Prior CI failure (arm_asan_ubsan, azure, sequential) is a pre-existing, unrelated crash

The single blocking failure on the previous commit (35d2de3) was a SEGV (STID 5743-5b74):

__asan_memset
ExpressionActions.h:48: Action
ExpressionActions.cpp:439: ExpressionActions::linearizeActions(...)
ExpressionActions.cpp:67:  ExpressionActions::ExpressionActions(ActionsDAG, ...)
InsertDependenciesBuilder.cpp:703: ExecutingInnerQueryFromViewTransform::process(...)

Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=108177&sha=35d2de313fdcc4aac0626d3d7a43e282f70fbfaf&name_0=PR&name_1=Stateless%20tests%20%28arm_asan_ubsan%2C%20azure%2C%20sequential%29

This is a heap-corruption victim, not the corruptor: ASan aborts in __asan_memset while linearizeActions does a push_back of a DAG node during the materialized-view insert pipeline build (InsertDependenciesBuilder.cpp:703). That code path manipulates ActionsDAG nodes only; it never touches the string column produced by trimLeft.

This PR changes only trim.cpp. The added fast path is read-only (it scans first bytes guarded by offsets[i] > prev_offset, performs no writes, and on the no-op case returns the input column unchanged), so it cannot corrupt the heap reached by the MV pipeline.

Evidence it is pre-existing and unrelated:

• The exact same crash signature (ExecutingInnerQueryFromViewTransform + SEGV in linearizeActions) hit 4 distinct unrelated PRs in the last 30 days, none of which touch trim.cpp (their topics: an ALTER/RENAME race fix, a SipHash pointer fix, AST-fuzzer oracles, and this PR). 0 hits on master in 60 days. The signature is rare and non-deterministic.
• On 2026-06-22 18:00 UTC this check had only these 2 rows fleet-wide (the SEGV and its Server died), so it was not an azure infra outage.

Local reproduction (clang-21 ASAN+UBSAN, -DSANITIZE=address,undefined) of the trim fast path is clean:

• Boundary cases (empty column, all-empty strings, single char, no-trim-needed, mixed first/late, empty-then-space, all-whitespace) and a 10M-row no-trim fast-path scan: no ASan reports.
• 20 concurrent trimLeft queries over mixed column shapes (200k rows each): clean, server survived.
• A materialized-view insert workload with trimLeft inside the MV query (50 inserts x 5000 rows, exercising the InsertDependenciesBuilder ExpressionActions path): clean, server survived.

Rebased onto current master and force-pushed to retrigger CI on a fresh base.

CI finished on c4499f5 (rebased base). The only red is Stress test (arm_asan_ubsan, s3), which is the chronic RefreshTask shutdown race, not caused by this PR (which only adds a read-only first-byte fast path in trim.cpp):

• Stress test (arm_asan_ubsan, s3) / Unexpected exception in refresh scheduling, STID 2508-34af (plus its "Cannot start clickhouse-server" / "Check failed" collateral from the same crashed run): chronic Context weak_ptr / RefreshTask shutdown race, 84 distinct PRs in 30d (last seen on other PRs minutes ago). Unrelated to trimLeft.

Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=108177&sha=c4499f5c720bf2f1188a3d506a86d86b09d8b97f&name_0=PR&name_1=Stress%20test%20%28arm_asan_ubsan%2C%20s3%29

The earlier one-off SEGV (STID 5743-5b74) was on the pre-rebase commit 35d2de3; it did not recur on this base, and local ASan+UBSan reproduction of the trim fast path (empty/single/all-whitespace/no-trim/mixed + 10M-row scan + concurrent + MV-insert workloads) was fully clean.

CI finished on HEAD c4499f5. All checks green except Stress test (arm_asan_ubsan, s3), which is a known chronic trunk issue unrelated to this string-function change:

• Logical error: 'Unexpected exception in refresh scheduling' (STID 2508-34af) + the consequent Cannot start clickhouse-server / Check failed cascade. This is the RefreshTask shutdown scheduling race (85 distinct PRs / 100 hits / 5 master in 30d), tracked separately; it is in the materialized-view refresh scheduler, not reachable from trimLeft/TrimImpl.

This PR only adds a fast path to default trimLeft (src/Functions/trim.cpp); no trim/string functional test failed. Ready for review.

Reworked per your proposal (3257ed3a3fc):

• Templates dropped. vectorImpl<L,R>, vectorFixedImpl<L,R>, executeImpl<L,R> and dispatch are gone. Three plain non-template methods now: vectorSpace, vectorCustom, vectorFixed. trim_left/trim_right are cheap runtime ifs in the loop body.
• Space and custom separated. Default whitespace trim is vectorSpace; the second-argument character set is vectorCustom.
• No resize in the loop. Capacity is reserved once (reserve_exact(input_data.size())); the loop writes into the reserved buffer and resize_exact(res_offset) sets the final size at the end.
• No find_first/last_not_symbols. The space path is a scalar while (begin < end && *begin == ' ') ++begin; / while (end > begin && end[-1] == ' ') --end;, then one memcpy of the kept bytes. This is what sidesteps the regressed NEON scan for the no-leading-space hot path. Right + space is symmetric.
• Custom left/right stay on the table-lookup path as you suggested, but now run after the early branch.

I dropped the original "return the input column unchanged" shortcut you flagged as too narrow: the scalar while loop already makes the common no-leading-space case fast (one byte compare per row) without a special case.

Validated: existing trim tests (00997_trim, 03224, 03315, 04327, 02711, 03722, 04236, 03890) plus 04401 all pass; results cross-checked against the replaceRegexpAll baseline over 5M randomized rows; ASan+UBSan clean on the full set and a 5M-row stress. ARM perf will be confirmed by the Performance Comparison job.

Pre-PR validation gate

Session id: cron:clickhouse-worker-slot-2:20260624-132300

@Algunenano reworked per the two comments.

Default-space path (vectorSpace): the loop now batches. It accumulates a run of consecutive rows that need no trimming and copies the whole run in a single memcpy; the run is broken only when a row actually needs a leading/trailing space removed, which is trimmed and copied on its own. When no row in the column needs trimming the entire output is one memcpy. Capacity is reserved once and the size is set at the end (no per-row resize).

Braces: added to every multi-line if body (the if + while + statement cases) in the space, custom, and FixedString paths.

Custom characters and FixedString keep the per-row path as you suggested.

Validation (debug + ASan/UBSan): trimLeft/Right/Both match replaceRegexpOne over 5M mixed rows and a 0..40 length sweep across the 16-byte memcpy boundary; all-clean, all-trimmed, alternating, trimmed-head/clean-tail and clean-head/trimmed-tail runs all clean; 00997_trim (100k random combos) and 04401 pass. ARM perf via the Performance Comparison job.

Pre-PR validation gate (round-2 rework)

Session id: cron:clickhouse-worker-slot-2:20260624-152800

trim_numbers is improved. trim_whitespaces was degraded

Reworked the default-whitespace path (commit c8966fe) along the three points raised:

1. Template parameters restored. dispatch resolves trim_left/trim_right back to compile-time <do_trim_left, do_trim_right> template parameters, so the inner loop has no per-row trim-direction branch. The previous revision's runtime branches were what regressed trim_whitespace.

2. SIMD scan restored for rows that need trimming. The rows in trim_whitespace carry ~17 spaces per side, so the scalar byte loop was the real slowdown. Rows that need trimming now use find_first_not_symbols<' '> / find_last_not_symbols_or_null<' '> again (16 bytes per instruction).

3. i/j while loop. The run is tracked with i = run start and j = current row, no separate open/flush bookkeeping. A cheap first/last-byte check decides run membership; when j hits a row that needs trimming, the run [i, j) is flushed in one memcpy, that row is SIMD-trimmed, and i resets. Output capacity is reserved once and sized at the end (no per-row reallocation).

The PR's fast path is preserved: a column with no leading/trailing spaces is copied in a single memcpy and never pays per-row trim work.

Local perf direction matches the prior CI run's improvements and the regression is gone (master-parity on trim_whitespace, ~1.4x faster on ARM trim_numbers). The CI Performance Comparison job on the new commit will confirm.

Validation: all 9 stateless trim tests pass (00997 = 100k random space combos, 04327 long custom char sets, 04401, 03890 FixedString). ASan+UBSan with abort_on_error=1: 5M-row cross-check of all three directions vs replaceRegexp (0 mismatches), boundary sweep around the 16-byte memcpy boundary, all-clean / all-trimmed / alternating runs, and 24 concurrent trim workloads, all clean.

Session id: cron:clickhouse-worker-slot-1:20260625-161500

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 112/114 (98.25%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 3 line(s) · Uncovered code

Full report · Diff report

CI finish ledger — c8966fe

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task whose fixing-PR link will be posted here when it opens. The PR's own diff (only src/Functions/trim.cpp + the 04401 test) does not touch Keeper, S3, or threading, so none of these are PR-caused.

Session id: cron:our-pr-ci-monitor:20260625-220000

Fixing PR for the test_keeper_snapshot_chunked_transfer/test.py::test_recover_* flaky (the Integration tests (amd_msan, 5/6) failure on this PR's commit c8966fe) created: #108557. Root cause: the recover subtests restart a SIGKILL'd follower whose snapshot recovery legitimately takes ~95 s under msan, but the test waited only start_clickhouse(20) (20 s). Not caused by this PR.

Thanks for the approval. Both follow-ups done:

PR description rewritten to match the final implementation:

• The changelog entry no longer says the input column is "returned directly" (the batching path always builds and returns the result column; the no-op case is copied in one batch memcpy).
• Scope corrected: it covers default-whitespace trimLeft/trimRight/trimBoth (+ aliases), not just trimLeft/ltrim.
• Closes: #106601 moved out of the template HTML comment so it is now an active relationship.

CI failures on c8966fe5 (the PR touches only src/Functions/trim.cpp + the new test, so none are PR-caused), all already reported and in flight:

• Integration tests (amd_msan, 5/6) / test_keeper_snapshot_chunked_transfer::test_recover_after_s3_read_error_during_transfer: flaky (26 hits / 7 unrelated PRs / 1 master in 30d), fix in Increase restart timeout in test_keeper_snapshot_chunked_transfer recover tests #108557.
• Stress test (arm_msan) and (arm_release) / Hung check failed, possible deadlock found: the chronic shutdown-deadlock family (Hung check failed, possible deadlock found #107941; 1053 hits / 464 PRs / 328 master in 30d), fix in Add timeout to waitBlobRemoval to prevent shutdown deadlock #101680 + Log live local thread pools at GlobalThreadPool shutdown #105905.

Nothing new to fix, so no separate PR needed. All 12 AMD/ARM Performance Comparison shards passed on this commit.