[JDBC] Connection#createArray & Connection#createStruct
#2523
+649
−241
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
|
chernser
requested review from
mzitnik and
Copilot
and removed request for
mzitnik
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
mzitnik
requested changes
Aug 17, 2025
|
|
||
| return listString.toString(); | ||
| } else if (x instanceof Object[]) { | ||
| StringBuilder arrayString = new StringBuilder(); | ||
| arrayString.append('['); |
There was a problem hiding this comment.
We can set it as a constant value. It is used widely in the code. same as ']'
|
@mzitnik I have an impression that createArrayOf() has no much benefit to use - just another wrapper. Besides documentation tells mostly nothing about multilevel arrays. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for Connection#createStruct and improves the existing Connection#createArray functionality in the ClickHouse JDBC driver. The changes add comprehensive support for creating SQL arrays and structures (tuples), enhancing JDBC compliance and enabling more sophisticated data type handling.
- Implements
Connection#createStructfor creating SQL Struct objects from ClickHouse tuples - Enhances
Connection#createArraywith better type validation, error handling, and support for nested arrays - Adds comprehensive test coverage for various data types in arrays and struct creation
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java |
Implements createStruct() and improves createArray() with proper validation |
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/Struct.java |
New Struct implementation for JDBC Struct interface |
jdbc-v2/src/main/java/com/clickhouse/jdbc/types/Array.java |
Enhanced Array implementation with validation and lifecycle management |
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java |
Utility improvements for array/list conversions and IP address handling |
jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java |
Refactored object encoding with support for Struct objects |
jdbc-v2/src/test/java/com/clickhouse/jdbc/ConnectionTest.java |
Comprehensive tests for createArray and createStruct functionality |
jdbc-v2/src/test/java/com/clickhouse/jdbc/DataTypeTests.java |
Tests for nested arrays and IP address type conversions |
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java |
Added methods to BinaryStreamReader.ArrayValue for better array handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| assertEquals(dst[2], src[2]); | ||
|
|
||
| assertNull(JdbcUtils.convertArray(null, int.class)); | ||
| assertEquals(JdbcUtils.convertArray(new Integer[] { 1, 2}, null), new Integer[] { 1, 2}); |
There was a problem hiding this comment.
The assertion will always fail because it's comparing object references, not array contents. Use assertArrayEquals instead of assertEquals for array comparison.
Suggested change
| assertEquals(JdbcUtils.convertArray(new Integer[] { 1, 2}, null), new Integer[] { 1, 2}); | |
| assert org.testng.Assert.assertTrue(Arrays.equals(JdbcUtils.convertArray(new Integer[] { 1, 2}, null), new Integer[] { 1, 2}), "Arrays are not equal"); |
Copilot uses AI. Check for mistakes.
| @@ -726,7 +728,7 @@ public void testIpAddressTypes() throws SQLException, UnknownHostException { | |||
| long seed = System.currentTimeMillis(); | |||
| Random rand = new Random(seed); | |||
|
|
|||
| InetAddress ipv4AddressByIp = Inet4Address.getByName(rand.nextInt(256) + "." + rand.nextInt(256) + "." + rand.nextInt(256) + "." + rand.nextInt(256)); | |||
| InetAddress ipv4AddressByIp = Inet4Address.getByName("90.176.75.97"); | |||
| InetAddress ipv4AddressByName = Inet4Address.getByName("www.example.com"); | |||
| InetAddress ipv6Address = Inet6Address.getByName("2001:adb8:85a3:1:2:8a2e:370:7334"); | |||
| InetAddress ipv4AsIpv6 = Inet4Address.getByName("90.176.75.97"); | |||
There was a problem hiding this comment.
[nitpick] The hardcoded IP address '90.176.75.97' replaces a random IP generation. Consider using a constant or commenting why this specific IP is used for testing.
Suggested change
| InetAddress ipv4AsIpv6 = Inet4Address.getByName("90.176.75.97"); | |
| InetAddress ipv4AddressByIp = Inet4Address.getByName(TEST_IPV4_ADDRESS); | |
| InetAddress ipv4AddressByName = Inet4Address.getByName("www.example.com"); | |
| InetAddress ipv6Address = Inet6Address.getByName("2001:adb8:85a3:1:2:8a2e:370:7334"); | |
| InetAddress ipv4AsIpv6 = Inet4Address.getByName(TEST_IPV4_ADDRESS); |
Copilot uses AI. Check for mistakes.
|
|
No saying about complex types, mainly for types coverage |
|
@mzitnik |
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
Connection#createStructConnection#createArrayCloses #2412
Closes #2360
Closes #2539
Checklist
Delete items not relevant to your PR: