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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create class for reading Json files in chunks #5530
Create class for reading Json files in chunks #5530
Changes from 17 commits
d1e4d8a
9ec0869
cfa2169
c1753f9
cc2ad30
32ec713
a9940e9
6f87583
0f75860
5c4269a
c469899
d0f9f5e
d3e6ab8
2d7cba8
4192d9b
f67239b
ca6e1d7
fa9639d
997f199
3e4146c
b403ed8
a1c4844
4ff0f7e
a9884ec
7a467d5
86d3524
c559e69
74b2e54
0c05eb8
a233b40
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Utf8JsonStreamReader
struct appears to have dual responsibilities violating single responsibility principle.Stream Wrapper: The struct acts as a wrapper around a stream, specifically for the purpose of reading it in chunks rather than loading the entire stream into memory.
Utf8JsonReader Wrapper: The struct also wraps the functionality of Utf8JsonReader. This includes functionalities like reading various data types (strings, integers, booleans) from JSON, handling different JSON token types, and managing the state of the JSON reader.
How about splitting it into 2 structs, one handles stream buffer, resizing the buffer etc., and another one wraps UTF8JsonReader methods such as GetString(), GetBoolean() etc.,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a large change if we want to do this I would like to do it in a new PR after the last one is complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some cases, which I currently don't understand, the STJ implementation raises an exception when
IsFinalBlock
is set to true. Therefore, it would be better to check this value first before invoking the underlyingRead
method. Another advantage is that readingIsFinalBlock
is an O(1) operation.https://github.com/dotnet/runtime/blob/3a5bea5d60ea04b897ac968a358ca99a1189d368/src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs#L269-L289
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we check for the final block first then we won't all the read at all. The exception you're seeing there means that the Utf8JsonReader was told it has all the data but the current property is none. None meaning that there is no JSON data in the reader. If we want to check for the scenario we can, I would think it should be in the constructor though not here.