jdbc-v2: an INSERT VALUES list holding a literal is routed to the beta RowBinary writer, which silently shifts the bound values
Nobody has claimed this yet.
Assessment
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Newbie friendliness
- 76/100
Research direction
Start at ConnectionImpl#prepareStatement and jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java, especially JavaCCParser#parsePreparedStatement and the useFunction handling described in the issue. Reproduce the listed literal, JDBC escape, and query-parameter cases with the beta writer enabled, then verify they use the generic path while placeholder-only values still use WriterStatementImpl and preserve the expected row data.
Written by the indexing model from the issue text.
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
- Enable
beta.row_binary_for_simple_insert=truewith the defaultJAVACCparser backend. - Create a table whose last column is nullable.
- Prepare an
INSERT ... VALUESwhose first value is a literal and whose remaining values are?, bind the
placeholders, and execute. - 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
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:
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):
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
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 toPreparedStatementImpl), and a values list
of placeholders only must keep being routed to the writer - that is the feature.
Configuration
Client Configuration
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:
mainatab256198a(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 TABLEstatements for tables involved:
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.
- Dominant language
- Java
- Stars
- 1.6k
- Forks
- 637
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 30
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from ClickHouse/clickhouse-java
-
Difficulty 1/5 Under an hour Newbie friendliness 85/100
ClickHouse/clickhouse-java#3111 ·
-
area:data-type bug
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
ClickHouse/clickhouse-java#3098 · 1 comment ·
-
bug client-api-v2 test
Difficulty 2/5 1-3 hours Newbie friendliness 92/100
ClickHouse/clickhouse-java#3076 ·
-
area:sql-parser bug client-v1
Difficulty 1/5 1-3 hours Newbie friendliness 92/100
ClickHouse/clickhouse-java#3066 ·
-
area:general bug client-api-v2 jdbc jdbc-v2
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
ClickHouse/clickhouse-java#3063 ·
All issues in ClickHouse/clickhouse-java
Similar issues
-
awaiting triage bug Causes friction Hop Gui P1 P2 Transforms
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
apache/flink-agents#1152 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 70/100
jenkinsci/blueocean-plugin#5417 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
objectionary/eo-graphs#75 ·