ClickHouse / ClickHouse/clickhouse-java

jdbc-v2: an INSERT VALUES list holding a literal is routed to the beta RowBinary writer, which silently shifts the bound values

Đang mở
#3,083 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
area:jdbc-insert area:sql-parser bug jdbc-v2
Ngôn ngữ chính
Java
Star
1.6k
Fork
636
Merge trung bình
2 ngày 23 giờ
Pull request đã merge (30 ngày)
29

Mô tả

## Description

With the beta RowBinary writer enabled (`beta.row_binary_for_simple_insert=true`) and the default `JAVACC` parser
backend, an `INSERT ... VALUES` list that is **not** placeholders only - it holds a literal, a JDBC escape sequence
(`{fn now()}`) or a ClickHouse query parameter (`{p1:DateTime}`) - is still routed to `WriterStatementImpl`, the
RowBinary writer.

That writer takes one value per column from the bound parameters, so a values list with fewer placeholders than
columns is written with the values shifted left. The literal is dropped and the trailing column receives no value.
When the trailing column is nullable the statement **succeeds and stores wrong data**; otherwise it fails with a
misleading error.

`ConnectionImpl#prepareStatement` selects the writer when the statement is a single-values-group `INSERT` that is not
an insert-from-select and does not use a function - the comment above that test states the intent is "a values list of
parameter placeholders only". `ParsedPreparedStatement.useFunction` only reports **function calls**, so a values list
holding non-placeholder values that are not function calls passes the test.

This is not #3027 (that one is `ANTLR4` only, and is about a function call the grammar cannot match; here the `JAVACC`
backend parses the statement cleanly and still reports no function). `INSERT INTO t VALUES (now(), ?)` is handled
correctly - a function call is detected and the statement goes to `PreparedStatementImpl`.

### Steps to reproduce
1. Enable `beta.row_binary_for_simple_insert=true` with the default `JAVACC` parser backend.
2. Create a table whose last column is nullable.
3. Prepare an `INSERT ... VALUES` whose first value is a literal and whose remaining values are `?`, bind the
placeholders, and execute.
4. Read the row back.

### Error Log or Exception StackTrace

Nullable trailing column - no error, wrong data (see below).

Non-nullable trailing column, `INSERT INTO bug_i (a, b, c) VALUES (7, ?, ?)` on `(a Int32, b Int32, c Int32)`:

```
java.sql.SQLException: java.lang.IllegalArgumentException: An attempt to write null into not nullable column 'c'
```

Type mismatch after the shift, `INSERT INTO bug_t (a, b) VALUES ({fn now()}, ?)` on `(a DateTime, b Int32)` with
`setObject(1, 42)`:

```
java.sql.SQLException: java.lang.IllegalArgumentException: Cannot convert 42 to DateTime
```

### Expected Behaviour

The values list is not placeholders only, so the statement must be given the generic parameter substitution path
(`PreparedStatementImpl`), as it is when the beta writer is disabled and as it is for `VALUES (now(), ?)`.

Expected result of the reproduction below, and what the standard path produces:

```
ROW: a=7 b=20 c=30
```

Actual result on the beta writer path - the literal `7` is dropped, `20` and `30` shift into `a` and `b`, and `c`
becomes null, with no error:

```
ROW: a=20 b=30 c=null
```

### Code Example

```java
Properties p = new Properties();
p.setProperty("beta.row_binary_for_simple_insert", "true");

try (Connection c = DriverManager.getConnection(url, p)) {
try (Statement s = c.createStatement()) {
s.execute("CREATE TABLE bug_n (a Int32, b Int32, c Nullable(Int32)) ENGINE MergeTree ORDER BY tuple()");
}
try (PreparedStatement ps = c.prepareStatement("INSERT INTO bug_n (a, b, c) VALUES (7, ?, ?)")) {
System.out.println(ps.getClass().getSimpleName()); // WriterStatementImpl
ps.setObject(1, 20);
ps.setObject(2, 30);
ps.executeUpdate(); // succeeds
}
try (Statement s = c.createStatement();
ResultSet rs = s.executeQuery("SELECT a, b, c FROM bug_n")) {
rs.next();
// expected 7, 20, 30 - actual 20, 30, null
System.out.println(rs.getString(1) + " " + rs.getString(2) + " " + rs.getString(3));
}
}
```

Affected values lists, all reported by the `JAVACC` backend as `useFunction=false`, `valueGroups=1`, and all routed to
`WriterStatementImpl`:

```
useFunction=false groups=1 args=2 | INSERT INTO bug_i (a, b, c) VALUES (7, ?, ?)
useFunction=false groups=1 args=2 | INSERT INTO bug_i (a, b, c) VALUES (?, ?, 7)
useFunction=false groups=1 args=1 | INSERT INTO bug_t (a, b) VALUES ({fn now()}, ?)
useFunction=false groups=1 args=1 | INSERT INTO bug_t (a, b) VALUES ({p1:DateTime}, ?)
useFunction=false groups=1 args=1 | INSERT INTO bug_t (a, b) VALUES ('2020-01-01 00:00:00', ?)
useFunction=true groups=1 args=1 | INSERT INTO bug_t (a, b) VALUES (now(), ?) <- correct, not routed
```

### Root cause

`jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java`, `JavaCCParser#parsePreparedStatement`.

Lines 108-114 already scan the values list for anything that is not `?`, `,` or whitespace and set `useFunction` for
it - a check that covers exactly the non-placeholder values above:

```java
for (int i = startIndex + 1; i < endIndex; i++) {
char ch = query.charAt(i);
if (ch != '?' && ch != ',' && !Character.isWhitespace(ch)) {
stmt.setUseFunction(true);
break;
}
}
```

Line 124 then **overwrites** that result unconditionally with the token manager's function flag, which is only set when
the grammar matches a function call (`ClickHouseSqlParser.jj:834`):

```java
stmt.setUseFunction(parsedStmt.isFuncUsed());
```

so the scan above has no effect and every non-function, non-placeholder value is reported as `useFunction=false`.

### Suggested fix

Do not discard the scan result - combine the two signals rather than overwriting, for example

```java
stmt.setUseFunction(stmt.isUseFunction() || parsedStmt.isFuncUsed());
```

Two notes for whoever takes this:

* The values-list positions the scan uses are themselves not always offsets into the original SQL for statements
containing a JDBC escape sequence or a `{p:T}` parameter - see #3017 / PR #3018. A guard that does not depend on
those positions, such as requiring the placeholder count to equal the column/schema count before selecting the
writer, would be robust on its own; the two fixes are complementary.
* `VALUES (now(), ?)` must keep its current behavior (detected, routed to `PreparedStatementImpl`), and a values list
of placeholders only must keep being routed to the writer - that is the feature.

### Configuration

#### Client Configuration
```java
Properties p = new Properties();
p.setProperty("beta.row_binary_for_simple_insert", "true");
// jdbc_sql_parser left at its default (JAVACC)
```

#### Environment
* [ ] Cloud
* Client version: `main` at `ab256198a` (0.11.0-rc1)
* Language version: OpenJDK 17
* OS: Ubuntu 24.04 (container)

#### ClickHouse Server
* ClickHouse Server version: 26.7.3.19
* ClickHouse Server non-default settings, if any: none relevant
* `CREATE TABLE` statements for tables involved:

```sql
CREATE TABLE bug_n (a Int32, b Int32, c Nullable(Int32)) ENGINE MergeTree ORDER BY tuple();
CREATE TABLE bug_i (a Int32, b Int32, c Int32) ENGINE MergeTree ORDER BY tuple();
CREATE TABLE bug_t (a DateTime, b Int32) ENGINE MergeTree ORDER BY b;
```

* Sample data: none needed, the reproduction inserts its own.

---

Found by automated analysis of jdbc-v2 while working on PR #3018, and verified end to end against a live ClickHouse
server rather than by inspection.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Bắt đầu từ ConnectionImpl#prepareStatement và jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java, đặc biệt là JavaCCParser#parsePreparedStatement và cách xử lý useFunction được mô tả trong issue. Tái hiện các trường hợp literal, JDBC escape và tham số truy vấn được liệt kê khi bật beta writer, sau đó xác minh rằng chúng sử dụng generic path, trong khi các giá trị chỉ chứa placeholder vẫn sử dụng WriterStatementImpl và giữ nguyên dữ liệu hàng như mong đợi.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
java, sql
Lĩnh vực
backend, databases
Loại issue
Lỗi
Độ khó
3/5
Thời gian dự kiến
1-2 ngày
Mức độ hoạt động
Sôi nổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
76/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.