Fix getTableSchemaFromQuery: strip trailing comment/semicolon before DESC wrapping (#2982) - #2984
Open
polyglotAI-bot wants to merge 3 commits into
Open
Fix getTableSchemaFromQuery: strip trailing comment/semicolon before DESC wrapping (#2982)#2984polyglotAI-bot wants to merge 3 commits into
polyglotAI-bot wants to merge 3 commits into
Conversation
getTableSchemaFromQuery wrapped user SQL verbatim in `DESC (<sql>) FORMAT TSKV`, so a trailing single-line comment (`--`, `#`) swallowed the wrapper's closing parenthesis and a trailing `;` landed inside the sub-query, both raising ServerException code 62 (SYNTAX_ERROR). In jdbc-v2 this made PreparedStatement.getMetaData() silently return 0 columns. Add ClickHouseUtils.stripTrailingCommentsAndSemicolons and apply it to the sub-query before wrapping; comment-like text inside string literals and quoted identifiers is preserved. Fixes: #2982
![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 fa27984. Configure here.
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
… '!' ClickHouse's lexer recognizes '#' as a single-line comment only when it is immediately followed by a space or '!' (shebang); '#' followed by anything else, or at end of string, is an ordinary token (invalid SQL). The helper stripTrailingCommentsAndSemicolons previously treated every '#' as a comment start, so a query such as 'SELECT 1 AS a#b' was silently rewritten to 'SELECT 1 AS a' before DESC wrapping, yielding column metadata for the wrong query. Tightened the '#' check to match the server and added regression cases (incl. contrast cases that fail without the fix, and a '#!' shebang case that must still be stripped).
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
…s scanner Convert the trailing-comment scanner from a for-loop that reassigns its counter to a while-loop, and extract single-/multi-line comment detection into small helpers. Behaviour is unchanged (all 49 ClickHouseUtilsTest cases pass); this clears the SonarCloud new-code smells on the PR (S127 loop-counter reassignment x4, S3776 cognitive complexity, S3626 redundant continue, S125 comment misdetection x2) that failed the Maintainability Rating quality gate.
|
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 #2982.
Client.getTableSchemaFromQuery(String sql, Map params)builds its introspection query by wrapping the user SQL verbatim:"DESC (" + sql + ") FORMAT TSKV". Nothing strips a trailing statement terminator or a trailing SQL comment, so:-- …or# …) swallows the wrapper's closing)(andFORMAT TSKV) →Unmatched parentheses;;(bare, or followed by a comment) ends up inside the sub-query parentheses, which the server rejects.Both raise
ServerExceptioncode 62 (SYNTAX_ERROR). In jdbc-v2 this surfaces throughPreparedStatementImpl.getMetaData(), which callsgetTableSchemaFromQuerybefore execution, catches the exception atWARN, and silently falls back to a placeholder — sogetMetaData().getColumnCount()returns 0 for any query whose text ends with a comment or a;(including the very commonSELECT … ;form).The fix strips trailing comments, whitespace, and statement terminators from the query before wrapping it, in an SQL-aware way so that comment-like text and semicolons inside string literals / quoted identifiers are preserved. The untouched original SQL is still used for table-name resolution.
Changes
clickhouse-data— new reusable helperClickHouseUtils.stripTrailingCommentsAndSemicolons(String), a forward scanner that reuses the existingskipQuotedString/skipSingleLineComment/skipMultiLineCommenthelpers (adding#single-line comments), returns everything up to the last significant token, and returns the input unchanged fornullor malformed SQL (unterminated quote/comment) so the server still produces its own error.client-v2—Client.getTableSchemaFromQuery(String, Map)now wraps the stripped sub-query instead of the raw SQL.Test
ClickHouseUtilsTest(unit,@DataProvider) — pins the helper across--/#/block trailing comments,;,;+comment,;\n+comment, empty--/#comment followed by more SQL, nested block comments, and contrast cases that must be left unchanged (interior comments, and--/#/;inside'…'and backtick identifiers), plus malformed-input passthrough.MetadataTests(client-v2 integration) —getTableSchemaFromQueryover the six reported query variants returns the singleUInt8columna; a contrast test proves a string literal containing-- …is not stripped.PreparedStatementMetaDataTest(jdbc-v2 integration, new) —PreparedStatement.getMetaData()before execution returns the real column (count 1, namea) for all six variants instead of an empty column list.Verified fail→pass: with the call site reverted to the verbatim wrap, the five trailing-noise variants fail with code 62 (baseline and the string-literal contrast still pass); with the fix, all pass. Existing
ClickHouseUtilsTestandMetadataTeststests are unchanged and still pass. (server 26.5)changes_checklist.md— String/SQL output changed: the introspection query text changes only for queries that previously failed; a query with no trailing noise is byte-identical (the helper returns the same instance), so this is additive (accepts input it previously rejected), not a behavior change for existing working queries. New method added: additive public static helper inClickHouseUtils, consistent with its sibling SQL scanners, with behavior-focused tests. Nodocs/features.mdchange (behavior restored, not a new/changed feature).Pre-PR validation gate
AGENTS.md/docs/changes_checklist.md