Fix jdbc-v2: skip // comments and heredocs when scanning for ? placeholders - #3010
Open
polyglotAI-bot wants to merge 7 commits into
Open
Fix jdbc-v2: skip // comments and heredocs when scanning for ? placeholders#3010polyglotAI-bot wants to merge 7 commits into
polyglotAI-bot wants to merge 7 commits into
Conversation
…olders SqlParserFacade.parseParameters knew '--'/'#' line comments, nested block comments and quoted strings, but not the '//' line comments and the $tag$...$tag$ heredocs that the server lexer also accepts. A '?' inside either was counted as a bind parameter, so PreparedStatement expected a value the application could not supply and executeQuery() failed with "Parameter at position 'N' is not set" for a query the server executes fine. The scan now treats '//' like the other line comment markers and skips a heredoc as an opaque token. A '$' is only a heredoc opener when it does not continue an identifier (a$b, a$x$), its tag contains word characters only, and a matching closing tag exists - otherwise it stays an ordinary character, matching the server lexer. Fixes: #3009
This was referenced Aug 3, 2026
Merged
This was referenced Aug 17, 2026
…eholder-scan-comments-heredocs
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…eholder-scan-comments-heredocs
…eholder-scan-comments-heredocs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d722e40. Configure here.
ClickHouseUtils.skipSingleLineComment returns len when the newline sits exactly at its startIndex, which is the case for an empty line comment (`--\n`, `//\n`, `#\n`) because callers pass the index after the marker. The placeholder scan then jumped to the end of the query and lost every later placeholder, so `SELECT ? //\n, ?` reported one parameter instead of two. The server ends such a comment at its newline and executes the rest of the query. Skip line comments with a local helper that starts at the second marker character, so an empty comment ends at its own newline. Applies to all three markers the scan handles (`--`, `//`, `#`/`#!`). Reported by Cursor Bugbot on PR #3010.
1 task
…eholder-scan-comments-heredocs # Conflicts: # jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java
…eholder-scan-comments-heredocs
|
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
1 participant
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.
Description
Fixes #3009.
SqlParserFacade.parseParametersis the linear scan that finds JDBC?placeholders (used by theJAVACCandANTLR4parser backends). It skipped quoted tokens,--/#line comments and nested/* */block comments, but it did not know two token kinds the server lexer also accepts://line comments and heredocs / dollar-quoted strings ($$...$$,$tag$...$tag$). A?inside either was therefore counted as a bind parameter, so thePreparedStatementexpected a value the application had no way to supply andexecuteQuery()failed withSQLException: Parameter at position 'N' is not setfor a query the server executes fine (verified on26.5:SELECT number FROM numbers(3) WHERE number = 1 // ?returns1,SELECT $$?$$returns?).The scan now treats
//like the other line-comment markers and skips a heredoc as an opaque token. Heredoc detection follows the server lexer: a$opens a heredoc only when it does not continue an identifier ($is a valid identifier character, soa$banda$x$are identifiers, not heredoc openers), the tag between the two dollar signs contains word characters only, and a matching closing tag exists; otherwise the$stays an ordinary character. The closing tag is searched non-greedily from the end of the opening tag, so$$a$b$$is one heredoc holdinga$b, matching the server.Nothing else changes:
--,#,#!, nested/* */, quoted tokens, the?::casthandling and division (4 / 2) behave exactly as before.Changes
jdbc-v2/.../internal/SqlParserFacade.java—parseParametersalso skips//line comments; newprivate static skipHeredoc(plus an ASCIIisWordCharhelper) skips$tag$...$tag$/$$...$$and returnsstartIndex + 1when there is no heredoc at that position.jdbc-v2/.../internal/BaseSqlParserFacadeTest.java— new parametrizedtestCommentsAndHeredocs(TestNG@DataProvider) pinning the placeholder count for//comments and heredocs, plus contrast cases that must keep their current behavior (//and$$inside string literals and comments, division,$in identifiers such asa$b/a$x$, an unterminated heredoc, and the already-supported--/#/#!/ nested block comments). It returns early for theANTLR4_PARAMS_PARSERbackend, which collects placeholders from the ANTLR grammar rather than from this scan (its lexer has no token for either kind — pre-existing and out of scope here, as is its existing gap on nested block comments).jdbc-v2/.../PreparedStatementTest.java— new integration testtestPlaceholdersWithCommentsAndHeredocsexercising the liveprepareStatement→setString→executeQuerypath, including the identifier contrast case.CHANGELOG.md— Bug Fixes entry.Test
Both tests were run against unpatched
mainfirst:SELECT 1 // ?→expected [0] but found [1],SELECT $tag$ ? $tag$ AS v→expected [0] but found [1],SELECT $$?$$, ?, $$?$$→expected [1] but found [3];java.sql.SQLException: Parameter at position '2' is not set.With the fix:
mvn -B -pl jdbc-v2 test→ 1376 tests, 0 failures;mvn -B -pl jdbc-v2 -DskipUTs=true -Dit.test=PreparedStatementTest verify→ 76 tests, 0 failures (ClickHouse26.5.1). Expected values were derived from the server, not from the client's current output.Pre-PR validation gate
main)PreparedStatement.executeQuery), not only on a helperAGENTS.md/docs/changes_checklist.md— change is confined tojdbc-v2; the new methods areprivate static(smallest visibility, no public API growth, so nodocs/features.mdchange); no configuration property, default, or exception type changed; per the conditional logic or guard changed and string, SQL, or serialized output changed sections the new branches are covered by focused parser regression tests including boundary inputs (empty tag$$$$, unterminated heredoc, end-of-string//, adjacent heredocs) and the previously-working inputs are pinned as contrast cases;CHANGELOG.mdupdated