Open
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } else if (value instanceof String) { | ||
| return (T) ((String)value).getBytes(StandardCharsets.UTF_8); | ||
| } else if (value instanceof InetAddress) { | ||
| return (T) ((InetAddress)value).getAddress(); |
There was a problem hiding this comment.
String/InetAddress handling ignores componentType parameter causing type mismatch
Medium Severity
The getPrimitiveArray method ignores the componentType parameter when handling String and InetAddress values, always returning byte[]. This causes a ClassCastException when calling methods like getIntArray(), getFloatArray(), etc. on columns containing String or InetAddress values. The String/InetAddress conversion to byte[] should only occur when componentType equals byte.class.
| } catch (Exception e) { | ||
| throw ExceptionUtils.toSqlState(String.format("Method: getBytes(\"%s\") encountered an exception.", columnLabel), String.format("SQL: [%s]", parentStatement.getLastStatementSql()), e); | ||
| } | ||
| return getBytes(getSchema().nameToColumnIndex(columnLabel)); |
There was a problem hiding this comment.
Missing closed check causes NPE on closed ResultSet
Medium Severity
The refactored getBytes(String columnLabel) method removed the checkClosed() call that existed in the old implementation. It now directly calls getSchema().nameToColumnIndex(columnLabel) before delegating to getBytes(int). Since getSchema() accesses reader.getSchema() and the close() method sets reader = null, calling this method on a closed ResultSet throws a NullPointerException instead of the expected SQLException with "ResultSet is closed" message.
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 adds support for reading and writing byte[] as binary strings in the JDBC driver, treating strings as byte arrays for compatibility with binary data operations.
Changes:
- Added
convertToUnhexExpression()utility to encodebyte[]asunhex(<hexstring>)SQL expressions forPreparedStatement#setBytes - Extended
ResultSet#getBytesand reader methods to return UTF-8 bytes of String values or raw address bytes of InetAddress objects - Refactored
AbstractBinaryFormatReaderto route column-name-based getters through index-based implementations for consistency
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java | Updated test to use plain SELECT ? instead of Array(Int8) cast for setBytes |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/JdbcDataTypeTests.java | Added integration tests for IP address byte retrieval and string/fixed-string round-tripping via setBytes/getBytes |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java | Introduced convertToUnhexExpression() to hex-encode byte arrays as SQL unhex expressions |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/ResultSetImpl.java | Swapped getBytes implementation to call index-based method from name-based variant |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java | Added byte[] encoding via unhex expression in encodeObject() |
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/AbstractBinaryFormatReader.java | Refactored to consolidate name-based getters onto index-based logic and added String/InetAddress→byte[] conversion in getPrimitiveArray() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+184
to
187
| try (PreparedStatement stmt = conn.prepareStatement("SELECT ?")) { | ||
| stmt.setBytes(1, new byte[] { 1, 2, 3 }); | ||
| try (ResultSet rs = stmt.executeQuery()) { | ||
| assertTrue(rs.next()); |
There was a problem hiding this comment.
The refactored testSetBytes no longer validates the returned value against the input byte[] { 1, 2, 3 }. Consider asserting that rs.getBytes(1) equals the original byte array to confirm round-trip correctness.
| for (String[] expected : testData) { | ||
| assertTrue(rs.next()); | ||
| assertEquals(new String(rs.getBytes("str"), "UTF-8"), expected[0]); | ||
| assertEquals(new String(rs.getBytes("fixed"), "UTF-8").replace("\0", ""), expected[1]); |
There was a problem hiding this comment.
Removing null bytes from FixedString in the assertion hides potential padding issues. Consider explicitly documenting or testing the expected padding behavior of FixedString(10) when storing shorter strings.
Suggested change
| assertEquals(new String(rs.getBytes("fixed"), "UTF-8").replace("\0", ""), expected[1]); | |
| String fixedValue = new String(rs.getBytes("fixed"), "UTF-8"); | |
| // FixedString(10) should be padded with null bytes up to length 10 | |
| assertEquals(fixedValue.length(), 10); | |
| StringBuilder padding = new StringBuilder(); | |
| for (int i = expected[1].length(); i < 10; i++) { | |
| padding.append('\0'); | |
| } | |
| assertEquals(fixedValue, expected[1] + padding.toString()); |
| hexChars[offset++] = HEX_ARRAY[v >>> 4]; | ||
| hexChars[offset++] = HEX_ARRAY[v & 0x0F]; | ||
| } | ||
| return new String(hexChars, StandardCharsets.UTF_8); |
There was a problem hiding this comment.
The hex characters are ASCII-only; using StandardCharsets.UTF_8 is correct but StandardCharsets.US_ASCII would be more semantically accurate for clarity.
Suggested change
| return new String(hexChars, StandardCharsets.UTF_8); | |
| return new String(hexChars, StandardCharsets.US_ASCII); |
| return null; | ||
| } | ||
| } catch (Exception e) { | ||
| throw ExceptionUtils.toSqlState(String.format("Method: getBytes(\"%s\") encountered an exception.", columnIndex), String.format("SQL: [%s]", parentStatement.getLastStatementSql()), e); |
There was a problem hiding this comment.
The error message uses %s for columnIndex (an int), which formats it as a string representation of the number. Consider using %d for clearer integer formatting.
Suggested change
| throw ExceptionUtils.toSqlState(String.format("Method: getBytes(\"%s\") encountered an exception.", columnIndex), String.format("SQL: [%s]", parentStatement.getLastStatementSql()), e); | |
| throw ExceptionUtils.toSqlState(String.format("Method: getBytes(\"%d\") encountered an exception.", columnIndex), String.format("SQL: [%s]", parentStatement.getLastStatementSql()), e); |
Comment on lines
+374
to
375
| throw new NullValueException("Column at index " + index + " has null value and it cannot be cast to " + | ||
| targetType.getTypeName()); |
There was a problem hiding this comment.
Error message states 'Column at index' but the index passed is 1-based (user-facing column index). Consider clarifying this is a column index (not 0-based array index) or use the column name for better UX.
Comment on lines
+512
to
+514
| } else if (value instanceof String) { | ||
| return (T) ((String)value).getBytes(StandardCharsets.UTF_8); | ||
| } else if (value instanceof InetAddress) { |
There was a problem hiding this comment.
The new logic to convert String and InetAddress to byte[] in getPrimitiveArray() is undocumented. Add a comment explaining these conversions support binary-string handling for JDBC getBytes().
Suggested change
| } else if (value instanceof String) { | |
| return (T) ((String)value).getBytes(StandardCharsets.UTF_8); | |
| } else if (value instanceof InetAddress) { | |
| } else if (value instanceof String) { | |
| // When a primitive byte[] is requested, allow String values to be exposed as their | |
| // UTF-8 binary representation. This supports JDBC ResultSet.getBytes() and other | |
| // binary-string handling that expects byte[] instead of String. | |
| return (T) ((String)value).getBytes(StandardCharsets.UTF_8); | |
| } else if (value instanceof InetAddress) { | |
| // Similarly, expose InetAddress values as their raw address bytes so that callers | |
| // using JDBC getBytes() or low-level binary APIs can consume the binary form. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
PreparedStatement#setBytesused -byte[]encoded asunhex(<hexstring>)that will let write binary stringsResultSet#getBytesused - thenbyte[]of String object will be returned. Note: still not optimal and internally logic will be changed to store strings asbyte[]and create them on demand.Checklist
Delete items not relevant to your PR:
Note
Medium Risk
Changes core value encoding/decoding paths for
byte[]and refactors many reader getters to flow through index-based logic, which could affect type conversions and null/error handling across result parsing.Overview
Adds first-class binary-string handling for
byte[]across the v2 client/JDBC stack.PreparedStatement#setBytesnow serializesbyte[]as a ClickHouseunhex('...')SQL expression via a newJdbcUtils.convertToUnhexExpression()helper.ResultSet#getBytesis updated to read bytes by column index/label through the binary reader, and the client-sideAbstractBinaryFormatReader#getByteArraycan now returnbyte[]not only for arrays but also when the underlying value is aString(UTF-8) orInetAddress(raw address bytes), alongside a broader refactor to route name-based getters through index-based implementations.Integration tests were extended to cover reading IP columns via
getBytes, inserting/retrieving string/fixed-string values usingsetBytes/getBytes, and adjustingtestSetBytessemantics away from treating bytes as anArray(Int8)parameter.Written by Cursor Bugbot for commit 492f659. This will update automatically on new commits. Configure here.