[Client-V2, JDBC] Fix problem reading JSON nested arrays #2601
Merged
[Client-V2, JDBC] Fix problem reading JSON nested arrays #2601
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
|
![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 (2)
- client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java (752-753) This test is tightly coupled to the implementation by directly casting to `BinaryStreamReader.ArrayValue`. Consider using a more generic approach that doesn't depend on specific implementation classes, such as verifying the behavior rather than the type.
-
clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseDataType.java (376-376)
The variable name was updated to `tmpBinTag2Type` for better readability, but the method call still uses the old name pattern. For consistency, this should be updated too.
tmpBinTag2Type.put(type.getBinTag(), type);
💡 To request another review, post a new comment with "/windsurf-review".
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java
Show resolved
Hide resolved
client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java
Outdated
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes issues reading data from JSON columns with nested arrays of objects or other multi-level nested objects in both the Client-V2 and JDBC components.
- Resolves problems with serialization and deserialization of complex JSON structures containing nested arrays
- Updates dynamic type handling for better support of nested data structures
- Adds comprehensive test coverage for various JSON nested array scenarios
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| jdbc-v2/src/test/java/com/clickhouse/jdbc/JdbcIntegrationTest.java | Modified test infrastructure methods to throw exceptions instead of returning booleans |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/DataTypeTests.java | Added comprehensive JSON nested array tests and fixed timezone handling in time tests |
| client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java | Added extensive dynamic type tests for nested structures, JSON, and variant types |
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java | Fixed Time64 serialization calculation and improved dynamic type tag handling |
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java | Comprehensive refactoring of dynamic data reading with proper handling of all data types |
| clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseDataType.java | Removed binary tags from geometric types and cleaned up imports |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java
Outdated
Show resolved
Hide resolved
client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java
Outdated
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java
Outdated
Show resolved
Hide resolved
mzitnik
requested changes
Sep 30, 2025
Contributor
mzitnik
left a comment
mzitnik
left a comment
There was a problem hiding this comment.
Also we did a lot of work related to other types do we have a good test coverage for that?
| } | ||
| return ClickHouseColumn.of("v", type, false, 0, 0); | ||
| case JSON: { | ||
| byte serializationVersion = readByte(); |
Contributor
There was a problem hiding this comment.
serializationVersion not in use? Seems to be redundant, or any other plans?
Contributor
Author
There was a problem hiding this comment.
I need to figure out. but seems not in use.
| } | ||
| case Nested: { | ||
| int size = readVarInt(input); | ||
| StringBuilder nested = new StringBuilder(); |
Contributor
There was a problem hiding this comment.
Since we have estimated size can we init with StringBuilder with size
Contributor
Author
There was a problem hiding this comment.
it will be a guessing. Default size of StringBuilder is 16 chars.
So I think we may define int DEFAULT_STRING_BUILDER_SIZE = 100 should be good.
Here is an example of tuple definition with total length of 103 chars/bytes:
Tuple(productId Int32, name String, price Float32, added Date, removed Date, storeId UUID, flags UInt8)
May be we need to make it configurable or may be add some metric first.
|
|
||
| assertTrue(rs.next()); | ||
| Object jsonObj = rs.getObject(1); | ||
| assertTrue(rs.next()); |
Contributor
There was a problem hiding this comment.
Should we test the content?
Contributor
Author
There was a problem hiding this comment.
Yes.
I was relying on tests in the client, but here is another story.
Will add checks.
chernser
commented
Sep 30, 2025
| createProperties.put(ClientConfigProperties.serverSetting("allow_experimental_time_time64_type"), "1"); | ||
| runQuery("CREATE TABLE test_time64 (order Int8, " | ||
| + "time Time('UTC'), time64 Time64(9) " | ||
| + "time Time, time64 Time64(9) " |
Contributor
Author
There was a problem hiding this comment.
This is the fix for falling tests on latest.
Contributor
Author
We had tests for other types. These changes are more code rearrangement to optimize multiple |
|
mzitnik
approved these changes
Oct 1, 2025
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
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 #2598
Closes #2593
Closes #2102
Closes #2613
Checklist
Delete items not relevant to your PR: