[jdbc] Fix prepared statement impl #2508
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves the PreparedStatementImpl implementation by adding comprehensive test coverage, implementing missing parameter validation, and introducing type casting functionality. The changes address issue #2418 by ensuring proper prepared statement behavior.
- Adds extensive test coverage for various prepared statement methods including streams, type casts, and parameter validation
- Implements validation to ensure all parameters are set before query execution
- Adds support for SQL type casting with ClickHouse data types
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| PreparedStatementTest.java | Comprehensive test coverage for prepared statement functionality including streams, type casts, and parameter validation |
| PreparedStatementImpl.java | Core implementation changes including parameter validation, type casting, and improved stream handling |
| JdbcUtils.java | Added SQL-to-ClickHouse type mapping and tuple conversion utility |
| Tuple.java | Added equals and hashCode methods for proper object comparison |
| SQLUtils.java | Added utility method for escaping single quotes in SQL strings |
| DataTypeUtils.java | Updated date formatters to use 'uuuu' pattern for better year handling |
| ClickHouseDataType.java | Implemented SQLType interface to enable type casting functionality |
jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java
Outdated
Show resolved
Hide resolved
mzitnik
requested changes
Jul 29, 2025
| * Formatter for the DateTime type. | ||
| */ | ||
| public static DateTimeFormatter DATETIME_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss"); | ||
| public static DateTimeFormatter DATETIME_FORMATTER = DateTimeFormatter.ofPattern("uuuu-MM-dd HH:mm:ss"); |
Contributor
There was a problem hiding this comment.
Do you have a link or explanation for doing that, since you do not see that so often (using the format with uuuu)
Contributor
Author
There was a problem hiding this comment.
y if for year in era and u is for just year.
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (this == obj) { |
Contributor
There was a problem hiding this comment.
Let's check if obj is null
Contributor
Author
There was a problem hiding this comment.
ok. The null check is done on obj instanceof Tuple.
| String val = values[i]; | ||
| compiledSql.replace(p, p+1, val == null ? "NULL" : val); | ||
| posOffset += val == null ? 0 : val.length() - 1; | ||
| if (val == null) { |
Contributor
There was a problem hiding this comment.
A remark for before and after the string will be great here
Contributor
Author
There was a problem hiding this comment.
Previously we did not check if all parameters are set. This change just moved lines 122 and 123 below the check.
val is null only when we have not set any value. NULL will be a "NULL" string.
| @DataProvider(name = "testTypeCastsDP") | ||
| public static Object[][] testTypeCastsDP() { | ||
| return new Object[][] { | ||
| {100, ClickHouseDataType.Int8, ClickHouseDataType.Int8}, |
Contributor
There was a problem hiding this comment.
{100, ClickHouseDataType.Int8, ClickHouseDataType.Int8} according to method signutare testTypeCastsWithoutArgument(Object value, SQLType targetType, ClickHouseDataType expectedType)
SQLType targetType -> ClickHouseDataType expectedType)
I'm missing something here
Contributor
Author
There was a problem hiding this comment.
I've made ClickHouseDataType to implement SQLType what is valid according to the spec.
I do always convert any type to ClickHouseDataType. In this case first ClickHouseDataType.Int8 acts as some SQLType instance. The second ClickHouseDataType.Int8 used to check if name of the type returned correctly. For that I use toTypeName function.
|
mzitnik
approved these changes
Jul 30, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2418
Closes #2391
Checklist
Delete items not relevant to your PR: