Fix {name:Type} hint scanner to follow the server lexer (#508)

[polyglotAI-bot]

Description

Fixes #508.

SqlParameterTypeExtractor scans the SQL text for {name:Type} hints, skipping comments and quoted tokens on the way. Its lexer only understood -- and # line comments, non-nested /* */ block comments, and single-quoted strings with '' as the only escape. The server lexer also accepts // line comments, nested block comments, backtick- and double-quoted identifiers, backslash escapes inside quoted tokens, and $tag$ ... $tag$ heredocs — and it treats # as a comment marker only when followed by a space or !. Because the two disagreed, hints were wrong in both directions: a {p:Type} sitting in text the client did not recognise as a comment was picked up as a real hint (throwing Parameter 'p' has conflicting type hints on a query the server accepts), and a --, # or ' inside a quoted identifier, heredoc or backslash-escaped string desynchronised the scanner so every real hint after it was silently dropped and the parameter fell back to CLR-type inference.

Every construct below was verified against a real server (26.5): SELECT 1 //x, SELECT 1 # x, SELECT 1 #!x are comments while SELECT 1 #x is a syntax error; /* a /* b */ still comment */ is one comment; `a--b`, "a--b", $$--$$ and 'a\'b' are single quoted tokens.

Changes

• ADO/Parameters/SqlParameterTypeExtractor.cs
  • // now starts a line comment; # starts one only when followed by ' ' or '!'.
  • Block comments nest — SkipBlockComment tracks depth instead of jumping to the first */.
  • New SkipQuotedToken handles single-quoted strings and backtick/double-quoted identifiers, honouring both doubling ('') and backslash (\') escapes. It is used both at the top level and for quoted tokens inside a type hint.
  • New TrySkipHeredoc skips $tag$ ... $tag$ (empty or ASCII-word-character tag, matching terminator required); a $ that does not open a terminated heredoc stays an ordinary character.
• CHANGELOG.md / RELEASENOTES.md entries.

Test

• SqlParameterTypeExtractorTests — three TestCaseSource groups: a hint inside a // comment, a nested block comment, a backtick/double-quoted identifier, a heredoc ($$ and $tag$) or a backslash-escaped string is ignored; a hint after any of those constructs is still extracted (including doubling escapes and a #/-- hidden inside them); and a $ that does not open a heredoc does not swallow the following hint.
• SqlParameterizedSelectTests — two integration cases through ClickHouseConnection/ClickHouseCommand against a real server, so the fix is pinned on the live runtime path: a conflicting hint hidden in a // comment, nested comment or heredoc no longer throws and the real hint is used; and a {val:DateTime64(3, 'UTC')} hint placed after `a--b`/"a'b"/$$--$$ is honoured (with the hint dropped the value is formatted as DateTime and the milliseconds are lost).
• Every new case fails on main and passes with the fix; the full ClickHouse.Driver.Tests suite is green (9670 passed).
• One existing test was rewritten rather than kept: ExtractTypeHints_ParameterInHashCommentNoSpace_IgnoresComment asserted that #{val:String} is a comment, which is the buggy behavior — the server rejects that query with code 62. It is now ExtractTypeHints_BareHash_NotTreatedAsComment and pins the correct behavior. No other test was changed.

Pre-PR validation gate

• Deterministic repro confirmed
• Root cause documented above
• Fix targets the root cause
• Test fails without fix, passes with fix
• No existing tests broken
• Convention compliance verified per AGENTS.md (NUnit TestCaseSource parametrization, integration tests against a real server, CHANGELOG + RELEASENOTES updated, no public API change)

[Copilot]

Pull request overview

This PR aligns the client-side {name:Type} hint scanner (SqlParameterTypeExtractor) with the ClickHouse server lexer so that parameter type hints are neither incorrectly picked up from comments/quoted tokens nor silently missed after lexer desynchronization.

Changes:

• Extended the SQL scanner to correctly skip // line comments, nested /* */ block comments, quoted identifiers (\...`, "..."), backslash escapes, and $tag$...$tag$heredocs; tightened#comments to only# /#!`.
• Added/updated unit tests to cover the newly supported lexical constructs and the corrected # behavior.
• Added integration tests to pin the end-to-end parameter binding behavior against a real server; updated changelog/release notes.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
ClickHouse.Driver/ADO/Parameters/SqlParameterTypeExtractor.cs	Updates the lexer/scanner to match server behavior for comments, quoting, escapes, and heredocs during {name:Type} extraction.
ClickHouse.Driver.Tests/ADO/SqlParameterTypeExtractorTests.cs	Adds unit coverage for ignored hints inside comments/quoted tokens and ensures hints after those constructs are still found.
ClickHouse.Driver.Tests/SQL/SqlParameterizedSelectTests.cs	Adds integration coverage confirming real parameter typing behavior is correct end-to-end.
CHANGELOG.md	Documents the user-visible bug fix for issue #508.
RELEASENOTES.md	Mirrors the changelog entry for the same bug fix.

[polyglotAI-bot]

Thanks — I checked this and I'm keeping the current form; the "hot path" premise doesn't hold for this branch.

Where the allocation actually happens: TrySkipHeredoc returns false before the Substring unless a complete $tag$ opener is present (the sql[i] != '$' guard on the line above). So there is no allocation for an ordinary $ in a query, and no allocation at all for the overwhelmingly common case of SQL containing no heredoc. When one is present, the cost is a single short string (a heredoc tag is a handful of ASCII word chars) per heredoc opener.

Where the real allocations are: ExtractTypeHints is called once per command (ClickHouseParameterCollection.ResolveTypeNames), and on that call it unconditionally allocates the result Dictionary plus, for every extracted hint, two Substrings and two Trims in TryExtractParameter. One optional short tag string on a rare branch is noise next to the allocations the method makes by design on every invocation, so removing it wouldn't move a measurable needle.

Why I'd rather not churn it: the change is behaviour-identical by construction, which means it cannot be pinned by a regression test — it would be an unverifiable refactor of a lexer that currently has a green 18-check matrix and full coverage of this path. Our policy is not to push behaviour-neutral micro-optimisations to a reviewed lexer for that reason.

That said, the allocation-free form is a one-liner if a maintainer wants it (the driver targets net6.0+, so MemoryExtensions.IndexOf is available): replace the Substring + string.IndexOf pair with sql.AsSpan(i + 1).IndexOf(sql.AsSpan(startIndex, i - startIndex + 1)) and offset the result. Happy to apply that on request — leaving this thread open for the maintainer to decide.

[codecov]

Codecov Report

❌ Patch coverage is 93.75000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...Driver/ADO/Parameters/SqlParameterTypeExtractor.cs	93.75%	1 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[alex-clickhouse]

The top-level scanner change looks right — I verified every construct against a live 26.6 server (//, # /#! vs. bare #x and #\t, nested block comments, backtick/double-quote identifiers with both '' doubling and \ escapes, heredoc tags restricted to ASCII word chars so $a-b$/$a b$/$ä$ are not heredocs, unterminated $tag$ as an ordinary identifier) and the implementation agrees with the server on all of them.

One gap worth closing before merge.

The TryExtractParameter half of the diff is untested

All 20 new unit cases and both integration tests exercise only the top-level scanner. The change to the inside-the-type scanner — swapping the inQuote loop for SkipQuotedToken — has no test at all.

It is not a no-op. I reverted just SqlParameterTypeExtractor.cs to main, kept the new tests, and ran three hints that are all valid SQL on 26.6:

Hint	main	this PR
{p:Enum8('a\'b' = 1)}	hint dropped entirely → silent fallback to CLR inference	Enum8('a\'b' = 1)
{p:Tuple(`a}b` UInt8)}	truncated at the inner } → wrong type	Tuple(`a}b` UInt8)
{p:Tuple("a}b" UInt8)}	truncated at the inner } → wrong type	Tuple("a}b" UInt8)

The first is the most user-visible: on main, a backslash-escaped quote inside an Enum hint leaves the old scanner believing it is still inside a string literal, so it runs off the end of the hint, returns (null, null, 0), and the parameter silently falls back to CLR-type inference. Server side these all round-trip fine:

SELECT {p:Enum8('a\'b' = 1)}                        param_p=a'b   -> a\'b
SELECT tupleElement({p:Tuple(`a b` UInt8)}, 'a b')  param_p=(5)   -> 5

Suggest adding a fourth TestCaseSource group in SqlParameterTypeExtractorTests, in the same style as the existing ones:

private static IEnumerable<TestCaseData> QuotedTokenInsideTypeHint()
{
    yield return new TestCaseData(@"SELECT {p:Enum8('a\'b' = 1)}", @"Enum8('a\'b' = 1)");
    yield return new TestCaseData("SELECT tupleElement({p:Tuple(`a}b` UInt8)}, 'a}b')", "Tuple(`a}b` UInt8)");
    yield return new TestCaseData("SELECT {p:Tuple(\"a}b\" UInt8)}", "Tuple(\"a}b\" UInt8)");
}

An integration case for the Enum8 one would be worth it too, since that path is the one that silently produces a wrong parameter type rather than throwing.

Minor, non-blocking

• SkipQuotedToken's unterminated return sql.Length is the only uncovered new line after running the full parameter test surface. It matters slightly more than a normal defensive branch: an unbalanced " or ` now swallows the rest of the query and drops every later hint, where before this PR both characters were transparent. Only reachable on SQL the server rejects anyway, but one negative case would pin it.
• Pre-existing, not for this PR: colonIndex = sql.IndexOf(':', startIndex + 1) is unbounded, so a { with no colon before its } steals the colon from the next parameter — "SELECT {a}, {b:Int32}" yields { "a}, {b" => "Int32" } and loses b. The server rejects {a} (Expected colon between name and type) so impact is limited to already-invalid SQL, but the XML doc on ExtractTypeHints advertises {name} as supported input, so the two disagree.
• Also pre-existing and a good follow-up issue: this PR fixes lexing in step 1 of the pipeline, but step 2 (ReplacePlaceholders -> StringExtensions.ReplaceMultipleWords) is a bare Regex.Replace with no quote/comment awareness. Against 26.6, SELECT 'user@val.com' AS lit, @val AS p with parameter val returns user{val:String}.com — the string literal is rewritten. That is the same bug class, but corrupting query text rather than dropping a hint.