Jdbc impl get result set in array #2668
Open
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 implements support for accessing array data through a JDBC ResultSet interface, addressing issues #1545 and #2683. The implementation adds a new ArrayResultSet class that allows arrays retrieved from ClickHouse to be navigated like a standard JDBC ResultSet, with proper type conversion and cursor navigation support.
Key changes:
- Added
ArrayResultSetimplementation with full JDBC ResultSet interface support - Integrated
ArrayResultSetinto existingArrayclass to enablegetResultSet()functionality - Created
ValueConvertersutility for type conversions between different Java types - Fixed Float/Double type mapping inconsistencies in JDBC type system
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java |
New class implementing JDBC ResultSet interface for array navigation with type conversion support |
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/Array.java |
Modified to return ArrayResultSet instances instead of throwing unsupported operation exceptions |
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java |
New utility class providing type conversion functions for various Java types |
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java |
Updated type mappings for Float and Time types, made arrayToObjectArray public |
jdbc-v2/src/test/java/com/clickhouse/jdbc/types/ArrayResultSetTest.java |
Comprehensive unit tests for ArrayResultSet functionality |
jdbc-v2/src/test/java/com/clickhouse/jdbc/ResultSetImplTest.java |
Integration tests verifying array ResultSet behavior with actual database |
jdbc-v2/src/test/java/com/clickhouse/jdbc/DataTypeTests.java |
Fixed float type assertions to use float literals instead of double |
jdbc-v2/src/test/java/com/clickhouse/jdbc/ConnectionTest.java |
Removed tests asserting getResultSet throws unsupported exception |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.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 (9)
-
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java (225-225)
Method name has a typo ('conveSql' instead of 'convertSql').
public Timestamp convertSqlTimestampToSqlTimestamp(Object value) { -
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java (107-107)
Update method reference to match the corrected method name.
mapBuilder.put(java.sql.Date.class, ImmutableMap.of(java.sql.Date.class, this::convertSqlDateToSqlDate, -
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java (109-109)
Update method reference to match the corrected method name.
mapBuilder.put(Time.class, ImmutableMap.of(Time.class, this::convertSqlTimeToSqlTime, -
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java (111-111)
Update method reference to match the corrected method name.
mapBuilder.put(Timestamp.class, ImmutableMap.of(Timestamp.class, this::convertSqlTimestampToSqlTimestamp, -
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java (95-99)
The checkColumnIndex method incorrectly validates column indices against the array length instead of the number of columns (which should be 2 - INDEX and VALUE). This can cause incorrect validation when the array length is less than the column index being checked.
private void checkColumnIndex(int columnIndex) throws SQLException { if (columnIndex < 1 || columnIndex > 2) { throw new SQLException("Invalid column index: " + columnIndex); } } -
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java (1229-1231)
The unwrap method returns null, which violates the JDBC specification. According to the JDBC spec, if the implementation doesn't support unwrapping to the requested interface, it should throw an SQLException.
@Override public <T> T unwrap(Class<T> iface) throws SQLException { if (iface.isAssignableFrom(getClass())) { return iface.cast(this); } throw new SQLException("Cannot unwrap to " + iface.getName()); } -
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java (503-506)
The previous() method implementation is incorrect. It calls absolute(pos) which would move the cursor to the current position, not the previous one. It should decrement the position before calling absolute().
@Override public boolean previous() throws SQLException { return absolute(pos); } -
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java (167-173)
When wrapping a checked exception, it's better to include the original exception message for better debugging.
public URL convertStringToURL(Object value) { try { return new URL((String) value); } catch (MalformedURLException e) { throw new RuntimeException("Failed to convert string to URL: " + value, e); } } - jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java (21-21) Duplicate import of `java.sql.Time`. This line should be removed.
💡 To request another review, post a new comment with "/windsurf-review".
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java
Outdated
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java
Outdated
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java
Outdated
Show resolved
Hide resolved
mzitnik
reviewed
Dec 9, 2025
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java
Show resolved
Hide resolved
Contributor
|
ValueConverters, according to the coverage test, have zero coverage. Is it real? |
Contributor
Author
|
@mzitnik coverage report is incorrect because tests for this class really in jdbc |
…d wrapping try-catch for exception while convertions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java
Outdated
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ValueConverters.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/ArrayResultSet.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
jdbc-v2/src/test/java/com/clickhouse/jdbc/types/ArrayResultSetTest.java:1
- Extra space before closing parenthesis. Remove the trailing space for consistent formatting:
if (i == 2)
package com.clickhouse.jdbc.types;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
mzitnik
approved these changes
Dec 10, 2025
Contributor
There was a problem hiding this comment.
let's open an issue to test it with AirByte @serebrserg
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
Array.getResultSet()andArray.getResultSet(index, countto improve GUI tools UX.com.clickhouse.jdbc.types.Array#getArray(long, int, java.util.Map<java.lang.String,java.lang.Class<?>>)because it involves more complex mapping and not widely used. Will do later.ArrayresultSetwith it. This way data conversion relies on value class knowledge and much faster than series ofinstanceofcalls.Closes #1545
Closes #2683
Checklist
Delete items not relevant to your PR: