Skip to content

Fix query type detection: a removed comment must still separate tokens - #929

Merged
joe-clickhouse merged 3 commits into
mainfrom
polyglot/comment-separator-classification
Aug 5, 2026
Merged

Fix query type detection: a removed comment must still separate tokens#929
joe-clickhouse merged 3 commits into
mainfrom
polyglot/comment-separator-classification

Conversation

@polyglotAI-bot

Copy link
Copy Markdown
Collaborator

Description

Fixes #928

remove_sql_comments builds the string that QueryContext.is_select, has_limit,
is_insert, is_command, the LIMIT 0 columns only probe and the DB-API bulk insert
check all run against. It replaced a removed comment with nothing. The server lexer
treats a comment as a token separator, so SELECT/*c*/number FROM numbers(9) is valid
SQL, but the classification string became SELECTnumber FROM numbers(9), the query
stopped looking like a SELECT, and the client side query_limit was silently not
applied (9 rows came back with query_limit=2). The same way,
SELECT number FROM numbers(9)/*c*/LIMIT 1 became numbers(9)LIMIT 1, has_limit was
False, the client appended its own LIMIT, and the server rejected the result with
Code: 62. Both forms are accepted by the server as written.

A -- line comment was never affected: the regex stops before the newline and the
newline is kept, so it separates its neighbors on its own.

Changes

  • clickhouse_connect/driver/query.py: a removed block comment is replaced with a single
    space. A line comment keeps returning nothing, its terminating newline is the separator.
  • clickhouse_connect/driver/_backend/httpcommon.py: columns_only_re hard coded a
    single space between LIMIT and 0. That space can now be the one left by a removed
    comment, so SELECT ... LIMIT /*c*/0 would have stopped reaching the columns only
    metadata probe and returned no column names. The regex accepts any whitespace there.
  • CHANGELOG.md: entry under UNRELEASED.

Test

  • tests/unit_tests/test_driver/test_parser.py::test_remove_comments_separates_tokens:
    parametrized matrix over the shapes a block comment can take (between two words, before
    a trailing LIMIT, between LIMIT and 0, inside INSERT INTO, multiline, two
    adjacent comments, leading and trailing), plus contrast rows pinning the unchanged
    behavior of line comments and of a /* */ sequence inside a quoted string or identifier.
  • tests/integration_tests/test_client.py::test_query_with_comment_between_tokens:
    end to end against a real server with query_limit=2, for both the sync and the async
    client via param_client / call. Fails on unpatched code with 9 rows instead of 2 and
    with Code: 62.
  • tests/integration_tests/test_client.py::test_get_columns_only_with_comment: pins the
    columns only probe for a comment before, inside and after the trailing LIMIT 0. The
    LIMIT /*c*/0 row is the regression guard for the columns_only_re change, it passes
    before this PR and would fail with the separator change alone.
  • test_remove_comments asserted the exact whitespace of the old output, which encoded the
    missing separator. Its expectation is updated to the same string with one space per
    removed block comment. No other existing test changed.
  • Full tests/unit_tests and tests/integration_tests runs match the pre-change baseline
    on this environment (the only failures are the pre-existing test_error_handling and
    alembic import errors and two unrelated test_client failures, identical with and
    without the patch). ruff format --check, ruff check and mypy are clean.

Checklist

  • Deterministic reproduction on main before the change
  • Root cause identified and explained
  • Fix matches the root cause, at the layer that builds the classification string
  • Regression test fails without the fix and passes with it, sync and async
  • No collateral damage, no existing test weakened
  • CHANGELOG.md updated in the same PR
  • No public API change

Notes

  • Related to but independent of Fix query type detection: scan comments with the server lexer rules #926, which fixes the comment and quoting forms this
    scanner does not recognize at all. This PR only changes the replacement text for a
    comment it does recognize. The two touch the same function, so whichever merges second
    needs a small rebase.
  • While checking the consumers of the comment stripped string I found a pre-existing
    weakness in dbapi/cursor.py::_try_bulk_insert: it takes the table name as everything
    up to the first space or paren, so it already mis-parses the legal
    INSERT INTO db . tbl VALUES (%s). That is a separate defect and is not touched here.

remove_sql_comments replaced a comment with nothing. The server lexer treats
a comment as a token separator, so SELECT/*c*/number FROM numbers(9) is valid
SQL, but the classification string became SELECTnumber, the query stopped
looking like a SELECT, and the client side query_limit was silently dropped.
Likewise a comment before a trailing LIMIT hid it, so the client appended a
second LIMIT and the server rejected the query with Code: 62.

A removed block comment now leaves a single space behind. A line comment ends
at its newline, which is kept, so it already separated its neighbors.

Because that space can now land between LIMIT and 0, the columns only probe
regex accepts any whitespace there instead of exactly one space, so
LIMIT /*c*/0 keeps returning column metadata.

Closes #928

@joe-clickhouse joe-clickhouse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically sound. Haven't seen customers hitting this yet but will merge.

@joe-clickhouse
joe-clickhouse merged commit fe5cb79 into main Aug 5, 2026
36 checks passed
@joe-clickhouse
joe-clickhouse deleted the polyglot/comment-separator-classification branch August 5, 2026 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

remove_sql_comments drops a block comment without a separator, so SELECT/*c*/1 is misclassified

2 participants