remove_sql_comments misses // and # comments, nested block comments, backtick identifiers and heredocs, so queries are misclassified

[polyglotAI-bot]

Summary

clickhouse_connect/driver/query.py remove_sql_comments strips comments with a single
regex that only understands -- line comments, non-nested /* */ block comments and
'/" quoting. The ClickHouse lexer also accepts //, # and #! line comments,
nests /* */, and has backtick-quoted identifiers, backslash escapes and $tag$
heredocs. The result feeds QueryContext.is_select, has_limit, is_insert,
is_command, the columns_only_re probe in _backend/httpcommon.py, and
dbapi.Cursor insert detection, so a comment the client does not understand
silently changes how the query is handled.

Verified against ClickHouse 26.5.1.882 and clickhouse-connect main (1.6.0).

Observed

With query_limit=2:

query	expected rows	actual
SELECT number FROM numbers(5) -- LIMIT 5	2	2 (correct)
SELECT number FROM numbers(5) // LIMIT 5	2	5, query_limit silently not applied
SELECT number FROM numbers(5) # LIMIT 5	2	5, query_limit silently not applied
SELECT number FROM numbers(5) // LIMIT 0	2	0 rows, the query is routed to the columns-only metadata probe
SELECT number FROM numbers(5) /* a /* b */ LIMIT 0 */	2	0 rows, the inner */ ends the comment and leaves LIMIT 0 behind
SELECT number AS + 'a--b' + FROM numbers(9) LIMIT 1	1	DatabaseError code 62, the real LIMIT 1 is eaten as a comment so the client appends a second LIMIT
SELECT number, $$--$$ AS tag FROM numbers(9) LIMIT 1	1	DatabaseError code 62, same cause
SELECT number FROM numbers(9) WHERE toString(number) != 'a\'b-- LIMIT 0' LIMIT 1	1	DatabaseError code 62, the backslash escaped quote ends the string early

Server side confirmation that all of these are comments or quoted tokens:

SELECT 1 //x    -> 1
SELECT 1 # x    -> 1
SELECT 1 #!x    -> 1
SELECT 1 #x     -> code 62, `#` alone is not a comment marker
SELECT 1 /* a /* b */ still comment */ -> 1
SELECT $$--$$   -> --
SELECT 'a\'b'   -> a\'b

Cause

comment_re = re.compile(r"(\".*?\"|\'.*?\')|(/\*.*?\*/|(--)[^\n]*$)", re.MULTILINE | re.DOTALL)
is both incomplete and, being built from .*?, the same backtracking shape that was
rejected in #906.

Suggested fix

Replace the regex with a single linear left to right scan that follows the server lexer:
--, //, # and #! line comments, nested /* */ block comments, '', "" and
backtick quoting with backslash and doubled quote escapes, and $tag$ heredocs. An
unterminated comment or quote can be passed through unchanged, since the server rejects
the query anyway.

[adelbordbari]

Hi! I’d like to take this on. I’ll replace the regex-based remove_sql_comments with a simple linear scanner that handles the ClickHouse comment and quoting cases mentioned above, and add regression tests for each one.

Happy to follow a different approach if you have one in mind.

[joe-clickhouse]

Hi @adelbordbari! Thanks for the offer. Please be sure to reuse the comment, quote, and heredoc helpers already in driver/binding.py. A small shared helper is fine if needed.

As for implementation, I'd like this to cover both comment removal and the classification decisions that use it. Correct comment removal alone will still allow a literal containing LIMIT 1 to suppress query_limit. Similarly, a commented-out LIMIT 0 can also send a normal query through the metadata-only path and consequently discard its rows.

So, at a high level the approach I'd like to see is:

1. Share the existing lexical rules across the affected SELECT, LIMIT, command, and metadata checks. Keep quoted contents opaque for classification, preserve token boundaries, and keep the existing remove_sql_comments output behavior. The SQL sent to the server needs to stay separate from the analysis.
2. Preserve the current query-limit policy for subqueries, parameter-binding behavior, INSERT detection, and inline INSERT payloads. ClickHouse allows keywords as identifiers, so every unquoted word named LIMIT can't automatically count as a clause so this should stay focused on lexical correctness.
3. Of course please add regression tests and make sure they'll fail on main and then pass with the fix. Please cover ignored limits, duplicate limits, false metadata-only routing, real LIMIT 0 queries that keep metadata, quoted keywords, and comment separators. Be sure to exercise both sync and async clients and the shared HTTP/chDB metadata decision.
4. You'll also need to do some before/after performance measurements for complete query preparation as depending on the implementation it will be important. Cover ordinary SQL, large quoted values, large and nested comments, malformed input, and unmatched heredoc candidates at increasing sizes. The previous attempts had both large-query slowdowns and quadratic suffix searches.

Please be sure to preserve the separator fix from #929 and the existing binding, raw/Arrow, INSERT, and semicolon behavior. Follow the server's exact lexical rules, include the changelog entry, and report the regression and performance results with the PR.

Let me know if you hav any questions along the way!