[clent-v2] Support Reading JSONEachRow#2823
Draft
chernser wants to merge 26 commits into
Draft
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
|
|
chernser
changed the title
Contributor
Author
|
@claude review. is it ready for human? |
|
Claude finished @chernser's task in 3m 43s —— View job PR Review: [client-v2] Support Reading JSONEachRowTasks:
FindingsHigh1. NPE in primitive numeric accessors for null JSON values — File: 2. File: 3. File: 4. File: Medium5. SonarCloud reports E Reliability Rating — The SonarCloud analysis flagged actual bugs. The NPE and datetime-parsing issues above are the most likely root cause. The quality gate must pass before merge. 6. 7. Duplicate imports in File: 8. Low9. Tests are missing for null-column behavior — 10. Human Review Instructions for Important ChangesA human reviewer should check the following before approving:
Verdict
The PR adds a well-structured JSON reading layer with good test coverage for the happy path. The architecture (factory pattern, parser abstraction, schema inference) is reasonable. However, there are correctness bugs that will surface in ordinary production use: NPEs on nullable columns from all primitive getters, |
|
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
General
Client V2 mainly supports binary formats. However there is a demand and practical need to support also formats from JSON family because of JSON popularity and effectiveness to represent complex structured data.
There is no specific JSON reader because any application can make a request via client and read input stream with favorite JSON parser. However creating such reader would help to bring JSON parsing to JDBC. As interface is already define only type mapping and some glue code is required.
Goal of this PR is to add harness for text format readers. New common interface class is created to let abstract readers. Dedicated interfaces for binary and text formats will have very specific methods.
Client has no intent to include all JSON parsing libraries and all classes are implemented in isolated way - they are not referenced by default.
New json reader has important part of the code that adopts primitive types to java ones. This conversion is required, for example, in JDBC for needs of ResultSetImpl.
Client Support
Client is the main component to implement JSON support. It should be in the style of extension or plug-in. No direct references to any JSON libraries should be. User will configure library instance and client should have a way to use it. Therefor next problems should be addressed:
There are two libraries we will support - GSON (https://github.com/google/gson) and Jackson (https://github.com/FasterXML/jackson). Both libraries has root class that accepts configuration and customized for user needs and both root classes create a parser or reader that is bound to
InputStream.JSON parser will be used in reader and in this case instantiation is an application task. In general text format support is not a goal for the client so no dedicated method for creating readers for non-ClickHouse formats. This solved problem with customization as parser is instantiated by user.
As both libraries create IO stream bound entities to work with JSON it will be convenient to provide sort of a factory. This class will become an abstraction that used to create a wrapper for JSON parsing library. Another abstraction is
JsonParserinterface that is used by reader to iterate thru rows.JDBC
JDBC Driver is often use when minimal custom code is expected and it is the place where we have to provide selection between JSON processing libraries. This should be implemented by providing class name of
JsonParserFactoryimplementation. It solves problem of instantiation. Besides user may specify own implementation class name if customization is required. Instantiation will be performed at connection creation phase.JDBC driver will create JSON reader if
JsonParserFactoryis defined.Checklist
Delete items not relevant to your PR: