ClickHouse / ClickHouse/clickhouse-connect

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

Open
#925 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
521
Forks
159
Avg merge
1d 7h
Merged PRs (30d)
46

Description

## 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.

Contributor guide

Open the contributing guide

Research direction

Start in clickhouse_connect/driver/query.py at remove_sql_comments and trace its consumers in QueryContext, _backend/httpcommon.py, and dbapi.Cursor. Reproduce the listed queries first, then add regression coverage for the comment, nested block, quoting, escape, and heredoc cases. Done means those queries are classified and limited as expected without changing valid quoted content.

Written by the indexing model from the issue text.

Assessment

Tech stack
python, sql
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.