Merged
Conversation
2 tasks
![windsurf-bot[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Other comments (1)
- jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/JdbcUtilsTest.java (62-63) The test for nested arrays is a good addition, but consider adding a test case for arrays with 3 or more dimensions to ensure the implementation handles deeper nesting correctly.
💡 To request another review, post a new comment with "/windsurf-review".
Comment on lines
+380
to
+381
| arrayDimensions = new int[Math.max(dimensions - stack.size() - 1, 1)]; | ||
| arrayDimensions[0] = srcList.size(); |
Contributor
There was a problem hiding this comment.
There's a potential issue with array dimensions handling. If the actual nesting level of the data exceeds the specified dimensions parameter, it could lead to runtime errors. Consider adding validation or handling for cases where the actual nesting is deeper than expected.
Contributor
Author
There was a problem hiding this comment.
what "potential" issue?
Comment on lines
+422
to
+423
| arrayDimensions = new int[Math.max(dimensions - stack.size() - 1, 1)]; | ||
| arrayDimensions[0] = java.lang.reflect.Array.getLength(value); |
Contributor
There was a problem hiding this comment.
Similar to the issue in convertList, the array dimensions calculation in convertArray might not handle all edge cases correctly. Consider adding more robust dimension detection or error handling for mismatched array structures.
Comment on lines
+371
to
389
| while (!stack.isEmpty()) { | ||
| ArrayProcessingCursor cursor = stack.pop(); | ||
|
|
||
| for (int i = 0; i < cursor.size; i++) { | ||
| Object value = cursor.getValue(i); | ||
| if (value == null) { | ||
| continue; // no need to set null value | ||
| } else if (value instanceof List<?>) { | ||
| List<?> srcList = (List<?>) value; | ||
| arrayDimensions = new int[Math.max(dimensions - stack.size() - 1, 1)]; | ||
| arrayDimensions[0] = srcList.size(); | ||
| T[] targetArray = (T[]) java.lang.reflect.Array.newInstance(type, arrayDimensions); | ||
| stack.push(new ArrayProcessingCursor(targetArray, value, srcList.size())); | ||
| java.lang.reflect.Array.set(cursor.targetArray, i, targetArray); | ||
| } else { | ||
| java.lang.reflect.Array.set(cursor.targetArray, i, convert(value, type)); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The stack-based processing approach could potentially cause a StackOverflowError for deeply nested arrays. Consider adding a maximum depth check or using an iterative approach that's less susceptible to stack overflow issues.
Contributor
Author
There was a problem hiding this comment.
This is a iterative approach. Stack used to organize state.
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
|
Client V1 CoverageCoverage Report
Class Coverage
|
dolfinus
reviewed
Nov 26, 2025
| ClickHouseColumn column = ClickHouseColumn.of("arr", "Array(Int32)"); | ||
| List<Integer> src = Arrays.asList(1, 2, 3); | ||
| Integer[] dst = JdbcUtils.convertList(src, Integer.class); | ||
| Integer[] dst = JdbcUtils.convertList(src, Integer.class, 1); |
Contributor
There was a problem hiding this comment.
Shouldn't there be a case with nested array as well?
Contributor
Author
There was a problem hiding this comment.
there is another test. But your are right.
We plan to revisit this code and will do more testing then.
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
2 participants
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
Fixes issue with nested arrays.
Closes #2457
Checklist
Delete items not relevant to your PR: