ClickHouse / ClickHouse/clickhouse-java

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

Offen Anfängerfreundlich
#3,066 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
area:sql-parser bug client-v1
Vorherrschende Sprache
Java
Sterne
1.6k
Forks
636
Ø Merge
2 T. 23 Std.
Gemergte PRs (30 T.)
29

Beschreibung

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

Beitragsleitfaden

Beitragsleitfaden öffnen

Rechercherichtung

Beginne in clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseUtils.java bei skipSingleLineComment und überprüfe ClickHouseUtilsTest.testSkipSingleLineComment. Füge den im Issue beschriebenen Fall eines leeren Kommentars hinzu, stelle sicher, dass Kommentare ohne Zeilenumbruch weiterhin len zurückgeben, und führe anschließend den fokussierten Utility-Test sowie die betroffenen client/JDBC-Tests aus; fertig bedeutet, dass Platzhalter nach einem leeren ---Kommentar entdeckt und gebunden werden.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
java, sql
Bereich
database
Issue-Typ
Bug
Schwierigkeit
1/5
Geschätzter Aufwand
1-3 Stunden
Aktivitätsstatus
Aktiv
Klarheit
Klar beschrieben
Anfängerfreundlichkeit
92/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.