Fixes List offset bug in Nested JSON reader #12060
Merged
Conversation
This file contains 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
karthikeyann
added
bug
vyasr
approved these changes
Nov 3, 2022
3 tasks
bdice
reviewed
Nov 3, 2022
| @@ -525,14 +525,15 @@ void make_device_json_column(device_span<SymbolT const> input, | |||
| auto parent_node_id = ordered_parent_node_ids[i]; | |||
| if (parent_node_id != parent_node_sentinel and node_categories[parent_node_id] == NC_LIST) { | |||
| // unique item | |||
| if (i == 0 || | |||
| if (i == 0 or | |||
| (col_ids[i - 1] != col_ids[i] or ordered_parent_node_ids[i - 1] != parent_node_id)) { | |||
| // scatter to list_offset | |||
| d_columns_data[original_col_ids[parent_node_id]] | |||
| .child_offsets[row_offsets[parent_node_id]] = ordered_row_offsets[i]; | |||
| } | |||
| // TODO: verify if this code is right. check with more test cases. | |||
There was a problem hiding this comment.
Is this TODO still needed? Or does this PR cover enough test cases that we now trust its correctness?
There was a problem hiding this comment.
will keep the TODO for now.
This PR has test cases to test the conditions. But it needs a few more test cases to be thorough.
bdice
approved these changes
Nov 3, 2022
|
rerun tests |
1 similar comment
|
rerun tests |
Codecov ReportBase: 87.47% // Head: 88.12% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## branch-22.12 #12060 +/- ##
================================================
+ Coverage 87.47% 88.12% +0.64%
================================================
Files 133 135 +2
Lines 21826 21997 +171
================================================
+ Hits 19093 19384 +291
+ Misses 2733 2613 -120
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@gpucibot merge |
rapids-bot bot
pushed a commit
that referenced
this pull request
Nov 16, 2022
This PR adds support for byte_range to be used in nested JSON parser for JSON Lines format (newline delimited JSON http://ndjson.org/) The record delimiter "New lines" are only expected at the end of each record. Newlines in middle of record or within quotes are not expected and will lead to unknown behaviour. The record delimiters are not context aware in this PR. This PR provides libcudf APIs, Cython APIs and python tests to enable byte range support. This will allow dask to do distributed/segmented parsing of JSON. No Dask changes Addresses part of #11843 Depends on #12060 Authors: - Karthikeyan (https://github.com/karthikeyann) Approvers: - Elias Stehle (https://github.com/elstehle) - Lawrence Mitchell (https://github.com/wence-) - Robert Maynard (https://github.com/robertmaynard) URL: #12017
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.
Description
Fixes List offset end last item write condition bug
If there is a list row followed by empty list in next row, the previous row's end is not written to offsets.
Checklist