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
- Dominant language
- Java
- Stars
- 1.6k
- Forks
- 636
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 28
Description
## 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.
Contributor guide
Assessment
This issue has not been assessed yet.