ClickHouse / ClickHouse/clickhouse-connect
remove_sql_comments misses // and # comments, nested block comments, backtick identifiers and heredocs, so queries are misclassified
- 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
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