[client-v2, jdbc-v2] Configurable type hinting #2476
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
|
Contributor
Author
|
/windsurf review |
There was a problem hiding this comment.
Pull Request Overview
This PR adds configurable type hinting for array conversions, fixes primitive array encoding in the JDBC driver, and extends the client v2 with support for user-defined type mappings.
- Introduces a
TYPE_HINT_MAPPINGclient option and propagates it throughClient.Builder,JdbcConfiguration, and binary readers. - Enhances
PreparedStatementImplto serialize Java arrays (primitive and object) correctly and updatesDataTypeTestswith fixed‐length arrays,setObjectinserts, and a new map‐with‐array‐values test. - Implements default type hint mapping (
Array → List) in JDBC and client binary readers.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| jdbc-v2/src/test/java/com/clickhouse/jdbc/DataTypeTests.java | Fixed random array sizes, added setObject inserts, new map-array test. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java | Added conversion from List<?> to java.sql.Array. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java | Provided default type-hint mapping and hooked into client builder. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java | Added support for primitive and object array encoding in SQL. |
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/AbstractBinaryFormatReader.java | Added default mapping constant for type hints in readers. |
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java | Consumes default type hints when reading arrays. |
| client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java | Defined TYPE_HINT_MAPPING config, parsing utilities implemented. |
| client-v2/src/main/java/com/clickhouse/client/api/Client.java | Extended builder and reader factories to accept type-hint mappings. |
Comments suppressed due to low confidence (1)
jdbc-v2/src/test/java/com/clickhouse/jdbc/DataTypeTests.java:928
- The test inserts two map columns (
mapandmap2) but only asserts the contents ofmap. Add assertions formap2retrieval to fully validate both inserted map types and improve coverage.
public void testMapTypesWithArrayValues() throws SQLException {
| } | ||
| return new Array(((BinaryStreamReader.ArrayValue) value).asList(), "Object", JDBCType.JAVA_OBJECT.getVendorTypeNumber()); | ||
| } else if (type == java.sql.Array.class && value instanceof List<?>) { | ||
| System.out.println(value); |
There was a problem hiding this comment.
Avoid using System.out.println for debug output in production library code; switch to the existing logger to prevent polluting standard output.
Suggested change
| System.out.println(value); | |
| logger.log(Level.FINE, value.toString()); |
...rc/main/java/com/clickhouse/client/api/data_formats/internal/AbstractBinaryFormatReader.java
Outdated
Show resolved
Hide resolved
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Other comments (3)
- jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java (274-278) The `defaultTypeHintMapping()` method currently hardcodes Array type to List.class, but according to the PR description, this should be configurable. Consider adding a mechanism to allow users to configure type mappings through JDBC properties rather than hardcoding them.
-
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/AbstractBinaryFormatReader.java (68-68)
This line is now very long and exceeds typical line length limits. Consider breaking it into multiple lines for better readability:
protected AbstractBinaryFormatReader(InputStream inputStream, QuerySettings querySettings, TableSchema schema, BinaryStreamReader.ByteBufferAllocator byteBufferAllocator, Map<ClickHouseDataType, Class<?>> defaultTypeHintMap) { - jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java (811-813) Consider removing the unnecessary empty line before the closing bracket on line 812 to improve code readability.
💡 To request another review, post a new comment with "/windsurf-review".
| case RowBinary: | ||
| reader = new RowBinaryFormatReader(response.getInputStream(), response.getSettings(), schema, | ||
| byteBufferPool); | ||
| byteBufferPool, typeHintMapping); |
Contributor
There was a problem hiding this comment.
The typeHintMapping field is initialized in the constructor but there's no null check when passing it to the binary format readers. If a client is created without specifying type hints, this could potentially cause NullPointerExceptions in the binary readers that receive this parameter.
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java
Outdated
Show resolved
Hide resolved
|
|
||
| // Check the results | ||
| // Insert using common java objects | ||
| final String INSERT_SQL = "INSERT INTO test_arrays VALUES ( 1, ?, ?, ?, ?)"; |
Contributor
There was a problem hiding this comment.
Both array insertions use the same order value (1). This could lead to unpredictable test results when selecting with ORDER BY order. Consider using a different order value for the second insertion, such as 2.
mzitnik
approved these changes
Jul 1, 2025
|
|
||
| /** | ||
| * Converts given string to key value pairs. | ||
| * |
Contributor
There was a problem hiding this comment.
It will be great if we have a simple example of the string
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
|
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
There is a problem when nested type contains array. As we might not know application preferences client returns
ArrayValue. This PR lets to configure what to return insteadListorObject[].Additionally this PR lets to configure other data type mapping where it is possible.
Also this PR fixes writing Arrays - there is an issue how primitive arrays are converted to a statement.
Closes #2340
Closes #2464
Checklist
Delete items not relevant to your PR: