ClickHouse / ClickHouse/clickhouse-java

clickhouse-data (v1): ClickHouseUtils.skipSingleLineComment jumps to end of query for an empty -- comment, dropping the rest of the SQL

Open
#3,066 0 comments 0 reactions 0 assignees View on GitHub
area:sql-parser bug client-v1
Dominant language
Java
Stars
1.6k
Forks
636
Avg merge
2d 16h
Merged PRs (30d)
28

Description

## Description

`ClickHouseUtils.skipSingleLineComment` returns `len` (end of string) instead of the
index after the newline when the line comment is **empty** — i.e. when the newline sits
exactly at `startIndex`, as in `--\n`.

`clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseUtils.java:1151`

```java
public static int skipSingleLineComment(String args, int startIndex, int len) {
int index = args.indexOf('\n', startIndex);
return index > startIndex ? index + 1 : len; // strict '>' is the defect
}
```

Its javadoc says it returns "index of start of next line, right after `\n`". Almost every
caller passes `i + 2` (the character right after the 2-char `--` marker), which for an
empty comment is exactly the newline position, so `indexOf` returns `startIndex`, the
`index > startIndex` test is false, and the scan jumps to the end of the query. Everything
after the empty comment is skipped.

Affected v1 callers that pass `i + 2`:
* `clickhouse-jdbc/src/main/java/com/clickhouse/jdbc/JdbcParameterizedQuery.java:59` (`?` placeholder scan)
* `clickhouse-client/src/main/java/com/clickhouse/client/ClickHouseParameterizedQuery.java:126` and `:254` (named `:param` scan)
* `clickhouse-client/src/main/java/com/clickhouse/client/ClickHouseRequest.java:131`
* internal scanners in `ClickHouseUtils` itself (lines 1105, 1222, 1263, 1319, 1428, 1475, 1507, 1582)

`ClickHouseUtils.getLeadingComment` (line 1028) passes the marker start index `i`, not
`i + 2`, so it is not affected.

### Steps to reproduce
1. Build `clickhouse-data`, `clickhouse-client` and `clickhouse-jdbc` at `main` (40464dd).
2. Parse a query that contains an empty `--` line comment between two placeholders.
3. Observe that only the first placeholder is found and the rest of the query is dropped.

### Error Log or Exception StackTrace

```
== helper directly ==
skipSingleLineComment("a--\nb", 3, 5) = 5 (expected 4)
skipSingleLineComment("a-- x\nb", 3, 7) = 6 (correct)

== clickhouse-jdbc v1 JdbcParameterizedQuery ==
"SELECT ? --\n, ?" params=1 applied=SELECT 1 --\n, ? <-- wrong
"SELECT ? -- x\n, ?" params=2 applied=SELECT 1 -- x\n, 2 <-- correct
"SELECT ?, ?" params=2 applied=SELECT 1, 2 <-- correct

== clickhouse-client ClickHouseParameterizedQuery (named) ==
"SELECT :a --\n, :b" params=[a] applied=SELECT 1 --\n, :b <-- wrong
"SELECT :a -- x\n, :b" params=[a, b] applied=SELECT 1 -- x\n, 2 <-- correct
```

The second placeholder is never registered, so the emitted SQL keeps a literal `?` / `:b`,
and binding it through `PreparedStatement` fails with an out-of-range parameter index.

### Expected Behaviour

An empty `--` comment ends at its newline, exactly like a non-empty one. The server agrees
(ClickHouse 26.7.3.19):

```
$ printf 'SELECT 1 --\n, 2' | curl -s --data-binary @- http://localhost:8123/
1 2
```

So `SELECT ? --\n, ?` has two parameters, not one.

### Code Example

```java
ClickHouseConfig cfg = new ClickHouseConfig();
JdbcParameterizedQuery q = JdbcParameterizedQuery.of(cfg, "SELECT ? --\n, ?");
System.out.println(q.getParameters().size()); // prints 1, expected 2

StringBuilder sb = new StringBuilder();
q.apply(sb, new Object[] { 1, 2 });
System.out.println(sb); // prints "SELECT 1 --\n, ?", expected "SELECT 1 --\n, 2"
```

### Suggested fix

One character in `ClickHouseUtils.skipSingleLineComment`, plus a data row in
`ClickHouseUtilsTest.testSkipSingleLineComment`:

```java
int index = args.indexOf('\n', startIndex);
return index >= startIndex ? index + 1 : len; // index == -1 (no newline) still returns len
```

`indexOf` returns either `-1` or a value `>= startIndex`, so `>=` keeps the
unterminated-comment case (`-1`) returning `len` unchanged, and only changes the
newline-at-startIndex case. Contrast case that must keep its current behavior: a comment
with no newline at all (`SELECT ? --`) still scans to the end of the query.

Unlike #3035 and #3037, this needs no parser redesign — it is a single comparison operator
in one shared helper, so it may be worth taking even though V1 is in maintenance.

### Configuration

#### Environment
* [ ] Cloud
* Client version: 0.10.0-rc1-SNAPSHOT (`main`, 40464dd)
* Language version: JDK 17
* OS: Linux (Docker)

#### ClickHouse Server
* ClickHouse Server version: 26.7.3.19
* Non-default settings: none
* No tables required — reproduces with literal `SELECT`.

---
Found by automated analysis of the client while working on the jdbc-v2 placeholder scan
(#3009 / PR #3010), and verified here against a live server, not by inspection alone.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.