fixed issue converting multidimensional lists and arrays#2735
Open
fixed issue converting multidimensional lists and arrays#2735
Conversation
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
Contributor
Author
|
/cursor review |
There was a problem hiding this comment.
Pull request overview
Improves handling and test coverage for converting multidimensional lists/arrays (particularly nested arrays) and validates string/object/list representations in both JDBC and client modules.
Changes:
- Updates internal conversion logic to track nesting depth explicitly during multidimensional conversions.
- Expands integration tests for nested arrays across multiple element types and dimensions.
- Adds an additional JDBC assertion for retrieving a deep nested array via
ResultSet.getArray().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| jdbc-v2/src/test/java/com/clickhouse/jdbc/JdbcDataTypeTests.java | Adds an assertion for getArray() on deep nested arrays (but removes the explanatory issue comment). |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java | Refactors multidimensional conversion to use an explicit depth field instead of computing remaining dimensions from stack size. |
| client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java | Adds integration coverage for nested array types validating getString(), getObject(), and getList(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * getString() on nested arrays was failing with NullPointerException due to re-entrancy bug | ||
| * in DataTypeConverter when converting nested arrays to string representation. | ||
| */ | ||
| @Test(groups = { "integration" }) |
There was a problem hiding this comment.
The Javadoc explaining the linked regression (issue #2723 and the prior NPE/re-entrancy context) was removed. Since this test reads like a regression test, keeping a short comment with the issue link and the failure mode would help future maintainers understand why these exact assertions exist.
Suggested change
| @Test(groups = { "integration" }) | |
| @Test(groups = { "integration" }) | |
| /** | |
| * Regression test for nested array stringification and {@code getString()} on | |
| * array-typed expressions (see issue #2723). | |
| * <p> | |
| * Originally, calling {@code ResultSet.getString()} on: | |
| * <ul> | |
| * <li>nested array columns such as {@code Array(Array(Int32))}, and</li> | |
| * <li>array-returning expressions used inside {@code CASE/WHEN}</li> | |
| * </ul> | |
| * could throw a {@link NullPointerException} due to re-entrancy issues in | |
| * the driver. This test verifies that {@code getString()} now works | |
| * correctly for those scenarios. | |
| */ |
Comment on lines
+1271
to
+1272
| Array arr = rs.getArray(1); | ||
| assertEquals(arr.getArray(), new String[][][] {{{"a", "b"}, {"c"}}, {{ "d", "e", "f"}}}); |
There was a problem hiding this comment.
Two issues here: (1) rs.getArray(1) is index-based and may not refer to deep_nested if the SELECT column order changes; prefer rs.getArray(\"deep_nested\") for stability. (2) assertEquals(arr.getArray(), new String[][][] {...}) is likely to fail because multi-dimensional arrays usually require deep comparison; use a deep equality check (e.g., Arrays.deepEquals) or compare a deep string representation. Also consider freeing the java.sql.Array (e.g., arr.free()) to avoid resource leaks in integration tests.
Suggested change
| Array arr = rs.getArray(1); | |
| assertEquals(arr.getArray(), new String[][][] {{{"a", "b"}, {"c"}}, {{ "d", "e", "f"}}}); | |
| Array arr = rs.getArray("deep_nested"); | |
| String[][][] actual = (String[][][]) arr.getArray(); | |
| String[][][] expected = new String[][][] {{{"a", "b"}, {"c"}}, {{"d", "e", "f"}}}; | |
| assertTrue(Arrays.deepEquals(actual, expected)); | |
| arr.free(); |
Comment on lines
+424
to
426
| int depth = cursor.depth - 1; | ||
| arrayDimensions = new int[depth]; | ||
| arrayDimensions[0] = srcList.size(); |
There was a problem hiding this comment.
This can throw at runtime when depth becomes 0 (or negative): new int[0] is legal, but then arrayDimensions[0] = ... will always throw ArrayIndexOutOfBoundsException. The previous implementation clamped the array length to at least 1. Consider restoring a clamp/guard (e.g., ensure the dimension array has length >= 1) and/or throwing a clear exception when the actual nesting exceeds the declared dimensions.
Comment on lines
+467
to
469
| int depth = cursor.depth - 1; | ||
| arrayDimensions = new int[depth]; | ||
| arrayDimensions[0] = java.lang.reflect.Array.getLength(value); |
There was a problem hiding this comment.
Same problem as convertList: if cursor.depth is 1 when encountering a nested array, depth becomes 0 and indexing arrayDimensions[0] will throw. Please reintroduce a minimum length guard (or a deterministic error) so malformed or unexpectedly-shaped inputs don’t fail with an opaque ArrayIndexOutOfBoundsException.
| client.execute(tableDefinition(table, "rowId Int32", columnDef)).get(); | ||
|
|
||
| String insertSQL = "INSERT INTO " + table + " VALUES " + insertValues; | ||
| try (QueryResponse response = client.query(insertSQL).get()) {} |
There was a problem hiding this comment.
Using client.query(...) for an INSERT is unconventional and may add unnecessary overhead versus an execute-style API (if available in this module). If the client provides execute() / command() for non-SELECT statements, prefer that here to better match intent and reduce reliance on query semantics for DML.
Suggested change
| try (QueryResponse response = client.query(insertSQL).get()) {} | |
| client.execute(insertSQL).get(); |
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
Closes
Checklist
Delete items not relevant to your PR:
Note
Medium Risk
Touches core JDBC type-conversion logic for arrays; regressions could affect array reads across drivers, though changes are localized and backed by expanded integration coverage.
Overview
Fixes JDBC multidimensional array conversion by tracking remaining nesting depth per recursion frame in
JdbcUtils.convertList/convertArray, preventing incorrect dimension calculations when walking nested lists/arrays.Adds a comprehensive client integration test matrix for nested
Array(...)columns (2D–4D, strings, floats, and nullable elements) verifyingGenericRecord.getString,getObject(asArrayValue), andgetListoutputs, and extends the JDBC nested-array test to assertResultSet.getArray()returns the expected Java multidimensional array for deeply nested string arrays.Written by Cursor Bugbot for commit b75dd05. This will update automatically on new commits. Configure here.