[jdbc-v2] Base implementation of RowBinaryFormatWriter support in JDBC #2316

chernser · 2025-04-18T06:15:24Z

Summary

Adds support for RowBinaryFormatWriter and fixes some issues:

Implementation forks PreparedStatementImpl and uses RowBinaryFormatWriter for simple "INSERT INTO t VALUES ()" case
Implementation is simply collecting values as encoded bytes and writes out when batch is executed.

Additional work is needed further:

support "INSER INTO t (col1, col2) VALUES (?, ?)". It requires better parsing code.

Closes #2171

Checklist

Delete items not relevant to your PR:

Closes [jdbc-v2] JDBC Bulk Insert performance using RowBinary #2171
Unit and integration tests covering the common scenarios were added
A human-readable description of the changes was provided to include in CHANGELOG
For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

Copilot

Pull Request Overview

This PR provides a base implementation for RowBinaryFormatWriter support in JDBC and refactors PreparedStatement to handle batch insertions with enhanced parameter handling. Key changes include:

Adding tests and utility methods (e.g. unQuoteTableName) to support quoting operations.
Refactoring statement and prepared statement implementations for better batch handling and logging.
Updating RowBinaryFormatWriter to implement the new ClickHouseBinaryFormatWriter interface with additional tracking features.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/JdbcUtilsTest.java	Adds unit test for unQuoteTableName utility method
jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java	Refactors parameter count checks and adds batch insert tests
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java	Introduces unQuoteTableName method with regex pattern
jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java	Updates method accessibility and logging level improvements
jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java	Refactors SQL segmentation, parameter handling and batch insert logic
jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java	Adds condition for using the WriterStatementImpl for INSERT queries
client-v2/src/main/java/com/clickhouse/client/api/data_formats/RowBinaryFormatWriter.java	Implements ClickHouseBinaryFormatWriter with row count tracking and new value-setting methods
client-v2/src/main/java/com/clickhouse/client/api/data_formats/ClickHouseBinaryFormatWriter.java	Introduces a new interface for binary format writers

jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java

jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java

jdbc-v2/src/main/java/com/clickhouse/jdbc/WriterStatementImpl.java

jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java

jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java

mzitnik · 2025-04-20T14:30:10Z

jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java

+                        stmt.setInt(1, rnd.nextInt());
+                        stmt.setFloat(2, rnd.nextFloat());
+                        stmt.setInt(3, rnd.nextInt());
+                        stmt.addBatch();


We should also test with clearParameters() and also extending the data types

mzitnik · 2025-04-20T14:32:03Z

jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java

+                    "INSERT  INTO \n `%s` \nVALUES (?, multiply(?, 10), ?)", // only string possible
+                    "INSERT  INTO\n `%s` \nVALUES (?, ?, ?)", // row binary writer
+            };
+            Class<?>[] impl = new Class<?>[]{


Is it possible to add a JMH to test show check the differences in performance between two implementation (so we could explain why it is more beneficial to use the rowbinary )

yes it is possible. With a feature flag it is possible.

…erts under beta flag

Copilot

Pull Request Overview

This PR adds beta support for the RowBinaryFormatWriter in simple insert statements for the JDBC driver and fixes/improves related parsing logic. Key changes include updated statement parsing using the new StatementParser, introduction of a BETA_ROW_BINARY_WRITER driver property with accompanying implementation in PreparedStatementImpl and ConnectionImpl, and enhancements for the RowBinaryFormatWriter to manage row state and count.

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/StatementParserTest.java	Added tests for various SQL types and comment handling.
jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/JdbcUtilsTest.java	Added tests for unquoting table names.
jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java	Updated tests to cover parameter type changes and beta batch insert functionality.
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/StatementParser.java	Refactored SQL statement parsing logic and added table name extraction.
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/DriverProperties.java	Introduced new beta property for the row binary writer.
jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java	New prepareStatement logic conditionally returns WriterStatementImpl based on beta configuration.
client-v2/src/main/java/com/clickhouse/client/api/data_formats/RowBinaryFormatWriter.java	Updated RowBinaryFormatWriter to implement ClickHouseBinaryFormatWriter, managing row state and count.
client-v2/src/main/java/com/clickhouse/client/api/data_formats/ClickHouseBinaryFormatWriter.java	Added new interface for binary format writers.

Comments suppressed due to low confidence (2)

client-v2/src/main/java/com/clickhouse/client/api/data_formats/RowBinaryFormatWriter.java:90

Consider adding tests to cover the behavior of commitRow when no values have been set, ensuring that the rowCount is only incremented when a row is actually started.

if (rowStarted) {

jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java:370

Consider adding tests to verify that the beta writer path correctly selects WriterStatementImpl, and that the conditions (absence of a column list and function parameters) are properly validated.

if (config.isBetaFeatureEnabled(DriverProperties.BETA_ROW_BINARY_WRITER)) {

sonarqubecloud · 2025-04-30T06:16:21Z

Quality Gate failed

Failed conditions
1 Security Hotspot
44.9% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

mzitnik · 2025-04-30T06:47:34Z

jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java


-        if (this.statementType == StatementType.INSERT) {
+        if (this.statementType == StatementParser.StatementType.INSERT) {
            insertIntoSQL = originalSql.substring(0, originalSql.indexOf("VALUES") + 6);


See #2283 a reported probleam for indexOf("VALUES")

Yes, this is the problem. But with parser. This PR should not fix it.

Can you open an issues for the parser implementation

jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java

jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/StatementParser.java

Base implementation of RowBinaryWriter support in JDBC

Loading
Loading status checks…

52c6e6b

chernser requested review from mzitnik, Paultagoras and Copilot April 18, 2025 06:15

Copilot AI reviewed Apr 18, 2025

View reviewed changes

jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java Outdated Show resolved Hide resolved

mzitnik reviewed Apr 20, 2025

View reviewed changes

chernser added 3 commits April 22, 2025 12:44

fixed comment

7435503

Merge branch 'main' into jdbc_v2_row_binary_writer

b16f9c0

Fixed some parsing issues. Implemented RowBinaryWriter for simple ins…

b2fe6bb

…erts under beta flag

chernser requested review from Copilot and mzitnik April 23, 2025 22:22

Copilot AI reviewed Apr 23, 2025

View reviewed changes

chernser added 7 commits April 23, 2025 15:24

Merge branch 'main' into jdbc_v2_row_binary_writer

db3725c

added seed output for random

Loading
Loading status checks…

6d06aad

added JDBC benchmarks

c11a581

Merge branch 'main' into jdbc_v2_row_binary_writer

48ce367

changes after merge

Loading
Loading status checks…

8ecbe79

Merge branch 'main' into jdbc_v2_row_binary_writer

e50ce10

fixed returning correct information about execution result and resultset

Loading
Loading status checks…

0d2defc

mzitnik reviewed Apr 30, 2025

View reviewed changes

mzitnik approved these changes Apr 30, 2025

View reviewed changes

chernser merged commit 2a9ad55 into main Apr 30, 2025
24 of 25 checks passed

chernser deleted the jdbc_v2_row_binary_writer branch April 30, 2025 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jdbc-v2] Base implementation of RowBinaryFormatWriter support in JDBC #2316

[jdbc-v2] Base implementation of RowBinaryFormatWriter support in JDBC #2316

chernser commented Apr 18, 2025 •

edited

Loading

Copilot AI left a comment

mzitnik Apr 20, 2025

chernser Apr 23, 2025

mzitnik Apr 20, 2025

chernser Apr 23, 2025

Copilot AI left a comment

sonarqubecloud bot commented Apr 30, 2025

mzitnik Apr 30, 2025

chernser Apr 30, 2025

mzitnik Apr 30, 2025

[jdbc-v2] Base implementation of RowBinaryFormatWriter support in JDBC #2316

[jdbc-v2] Base implementation of RowBinaryFormatWriter support in JDBC #2316

Conversation

chernser commented Apr 18, 2025 • edited Loading

Summary

Checklist

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

mzitnik Apr 20, 2025

Choose a reason for hiding this comment

chernser Apr 23, 2025

Choose a reason for hiding this comment

mzitnik Apr 20, 2025

Choose a reason for hiding this comment

chernser Apr 23, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

sonarqubecloud bot commented Apr 30, 2025

Quality Gate failed

mzitnik Apr 30, 2025

Choose a reason for hiding this comment

chernser Apr 30, 2025

Choose a reason for hiding this comment

mzitnik Apr 30, 2025

Choose a reason for hiding this comment

chernser commented Apr 18, 2025 •

edited

Loading