Repository navigation
Fix parameters: rewrite @name placeholders in linear time - #697
Open
polyglotAI-bot wants to merge 2 commits into
Open
polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
At every @ in a code position, SqlPlaceholderRewriter compared the text with each placeholder key in turn, longest first. A multi-row INSERT has as many @ characters as parameters, so the rewrite took time quadratic in the row count (4,000 rows x 18 columns: about 24 s). Read the whole run of word characters after the @ and look it up once in the replacements dictionary. A name made only of word characters gets the same result as before. On a miss the scanner still skips only the @. The per-call longest-first sort is no longer needed and is gone. Fixes: #696 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
polyglotAI-bot
requested review from
alex-clickhouse and
mzitnik
as code owners
October 22, 2026 13:51
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The shared HTTP query path changes token matching, and validation across all supported target frameworks remains unconfirmed.
1 open issue
What changed in this PR
This PR addresses #696 by removing quadratic @name placeholder matching from the HTTP client’s parameterized-query path. The TCP client is unchanged.
Changes:
- Replaces sorted key scanning with whole-name parsing and a dictionary lookup.
- Adds a 72,000-parameter regression test and coverage for heredoc handling after an unmatched
@. - Adds a changelog fragment describing the performance improvement.
| File | Description |
|---|---|
ClickHouse.Driver/ADO/Parameters/SqlPlaceholderRewriter.cs |
Uses direct lookup instead of scanning every parameter name. |
ClickHouse.Driver.Tests/ADO/SqlPlaceholderRewriteTests.cs |
Adds large-query performance and heredoc regression coverage. |
changelog.d/696-placeholder-rewrite-speed.improvements.md |
Records the placeholder-rewriting speed improvement. |
Review effort: Balanced
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
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.

Summary
Fixes #696.
Root cause.
SqlPlaceholderRewriter.ReplacePlaceholdersscans the SQL once. At every@in a code position,MatchPlaceholdercompared the text with every key ofreplacements, longest first. In a multi-row parameterizedINSERT, both the count of@characters and the count of parameters grow with the row count, so the rewrite took O(rows²) time. Each call also sorted all keys again.Fix. At a
@, read the whole run ofIsWordCharcharacters after it and look up"@" + nameonce in the dictionary. The rewrite is now one pass. The longest-first sort is removed:@idcannot shadow@id_2, because the name is always the full run.Behavior.
_,$), the result is the same as before. The old code matched a key only when the next character was not a word character. For such a name, that is exactly the full run.@, so a$right after it can still open a heredoc.new Dictionary<string, string>(), ordinal and case-sensitive), so case handling did not change..,-, …) no longer matches. The server does not accept such a name: on 26.9,SELECT {a.b:Int32}fails withSYNTAX_ERROR("Expected colon between name and type"). Before,@a.bwas rewritten into that rejected form. Now@a.bis left as is, and the server rejects the@. If a parameteraalso exists,@a.bis now read as@afollowed by.b, the same way the server lexesa.b.Changes
ClickHouse.Driver/ADO/Parameters/SqlPlaceholderRewriter.cs: one dictionary lookup per@in place of the linear key scan, and no per-call sort.ClickHouse.Driver.Tests/ADO/SqlPlaceholderRewriteTests.cs:ReplacePlaceholders_MultiRowInsertWithManyParameters_RewritesAllQuickly: a 4,000-row x 18-columnINSERT(72,000 parameters, with names such asp1/p10/p100that are prefixes of each other). It checks the full rewritten SQL against an expected string built independently, and that the rewrite finishes in less than 3 s.Placeholderscase,heredoc after an unknown placeholder's at sign. It pins the miss path: if a miss skipped the whole name, this case would fail.changelog.d/696-placeholder-rewrite-speed.improvements.md.Test
main: fails, the rewrite took 28.9 s against the 3 s budget (the output itself was correct). With the fix: passes, about 0.2 s in isolation (0.26 s with coverlet).SqlPlaceholderRewriteTestscases, the new contrast case included, give the same results onmainand on this branch.dotnet test ClickHouse.Driver.Tests --framework net10.0 --filter "FullyQualifiedName~Parameter|FullyQualifiedName~Placeholder|FullyQualifiedName~Dapper|FullyQualifiedName~RowLimit|FullyQualifiedName~Sql": 4,559 passed, 0 failed, against a live 26.9 server. No existing test was changed.BenchmarkDotNet, ad-hoc and not committed.
ClickHouseParameterCollection.ReplacePlaceholders, net10.0, ShortRun:mainmain→ PR)WHEREwith 3 paramsThe new code allocates one
Substringper@. Small queries allocate less than before, because the sort buffers are gone. Large ones allocate about 5% more. AReadOnlySpan<char>alternate lookup could remove the substring on .NET 9+ only, but these numbers do not justify the extra#if.Checklist
Pre-PR validation gate
mainevery run (rewrite takes seconds instead of milliseconds)@linear key scan → one dictionary lookup)ClickHouseClient.BuildHttpRequestMessageWithQueryParamsand...WithFormDatacallClickHouseParameterCollection.ReplacePlaceholders@namerewrite.Related: ClickHouse/clickhouse-java#3249 has the same symptom in clickhouse-java, from a different mechanism.
🤖 Generated with Claude Code
Note
Medium Risk
Touches the live SQL rewrite path for every
@namequery; behavior is unchanged for typical names but differs for placeholders containing.or-.Overview
Fixes quadratic slowdown when rewriting many
@nameplaceholders (e.g. large multi-rowINSERTs) by replacing per-@scans over all parameter keys—and the per-call longest-first sort—with a single dictionary lookup after reading the full placeholder name.Matching now treats the name as the contiguous run of letters, digits,
_, and$after@. That preserves normal names likeid/id_2/id$xwhile changing edge cases:@a.bis no longer rewritten to a rejected{a.b:…}form (it becomes@aplus.bwhenaexists, or is left for the server to reject). On an unknown@, only the@is consumed so a following$can still start a heredoc.Adds a 72k-parameter timing regression test, a heredoc-after-miss case, changelog entry, and HTTP docs for placeholder naming.
Reviewed by Cursor Bugbot for commit c9de2e7. Bugbot is set up for automated code reviews on this repo. Configure here.