Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Simplify read_json by removing unnecessary reader/impl classes #9088

Merged
merged 34 commits into from
Nov 11, 2021
Merged

Simplify read_json by removing unnecessary reader/impl classes #9088

merged 34 commits into from
Nov 11, 2021

Conversation

cwharris
Copy link
Contributor

@cwharris cwharris commented Aug 21, 2021 •
edited
Loading

Depends on #9040

Removes the json reader and impl classes, replacing member variables with local variables, reduces cognitive overhead, and facilitates further refactoring.

cwharris added 25 commits August 14, 2021 05:08
@cwharris
simplify io/functions.cpp data source/sink factories
9c72e56
@cwharris
Merge branch 'branch-21.10' of github.com:rapidsai/cudf into io-funct…
884bde6 
…ions-simplify
@cwharris
remove filepath-related logic from csv and json readers
88e2399
@cwharris
remove filepath logic from avro, parquet, orc readers
62b9520
@cwharris
move range size padding calculation out of json/csv reader and in to …
fb01294 
…json/csv options
@cwharris
remove filepaths from json reader
d422aeb
@cwharris
replace json reader impl buffer member with local variable
e0cac1d
@cwharris
replace josn reader sources member with local variable
dc23634
@cwharris
delete useless json reader wrapper class
166c4d3
@cwharris
delete unused arrow namespace declaration
b3ce527
@cwharris
replace json reader_impl mr member with argument
5a997b8
@cwharris
replace json reader_impl options with local variable
89163fb
@cwharris
remove unused json_reader::allow_newlines_in_strings_ member
7ff3f1a
@cwharris
replace json reader_impl::opts_ with local variable
5c95398
@cwharris
remove json reader_impl byte_range members in place of local variables
0bde0b1
@cwharris
replace json reader_impl::dtypes_ with local variable
b4843f5
@cwharris
replace json reader_impl::metadata_ with local variable
f511e68
@cwharris
replace json reader_impl::data_ with local variable
5e307b5
@cwharris
replace json::reader_impl column_map members with local variables
f84dcc3
@cwharris
change json::reader_impl host buffer type from uint8_t to char
e11e9db
@cwharris
replace json::reader_impl uncomp data members with single span member
df10217
@cwharris
json::reader_impl simplify device data copy logic
0417be8
@cwharris
remove json::reader_impl::decompress_input function and inline the logic
f0fd5c1
@cwharris
replace json::reader_impl::uncomp_data_ member with local variable
45714bb
@cwharris
relocate json::reader_impl decompression code
8d18ffe
@github-actions github-actions bot added Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Aug 21, 2021
@cwharris cwharris added the cuIO cuIO issue label Aug 21, 2021
@vuule
Copy link
Contributor

vuule commented Sep 17, 2021

I think Elias and I can get it over the finish line, but not in 21.10. Since this is an internal refactoring, there's no reason to push for 21.10 anyway.

@vuule vuule mentioned this pull request Sep 28, 2021
Closed
@karthikeyann karthikeyann added the 2 - In Progress Currently a work in progress label Oct 8, 2021
@karthikeyann
Copy link
Contributor

karthikeyann commented Oct 8, 2021 •
edited
Loading

@cwharris moved to 21.12. Adding In-Progress label.

@karthikeyann karthikeyann changed the base branch from branch-21.10 to branch-21.12 October 8, 2021 16:33
@vuule vuule requested a review from elstehle October 27, 2021 21:33
vuule
vuule reviewed Oct 27, 2021
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just got a couple of questions.

cpp/src/hash/concurrent_unordered_map.cuh Show resolved Hide resolved
cpp/src/io/json/json_gpu.cu Show resolved Hide resolved
elstehle
elstehle reviewed Nov 1, 2021
View reviewed changes
Copy link
Contributor

@elstehle elstehle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost through with the review. Looks good to me so far, I like the refactoring.

cpp/src/io/json/json_gpu.cu Show resolved Hide resolved
@cwharris cwharris requested review from vuule and elstehle November 1, 2021 17:08
rgsl888prabhu
rgsl888prabhu approved these changes Nov 4, 2021
View reviewed changes
Copy link
Contributor

@rgsl888prabhu rgsl888prabhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just small set of questions or comments

cpp/include/cudf/io/detail/json.hpp Show resolved Hide resolved
cpp/src/io/json/json_common.h Show resolved Hide resolved
cpp/src/io/json/reader_impl.cu Show resolved Hide resolved
cpp/src/io/json/reader_impl.hpp Show resolved Hide resolved
mythrocks
mythrocks approved these changes Nov 4, 2021
View reviewed changes
Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. I'm 👍 on the change. Just a few minor questions.

elstehle
elstehle approved these changes Nov 6, 2021
View reviewed changes
Copy link
Contributor

@elstehle elstehle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; nothing more to add. Would just ask to address the other's comments before merging.

@jrhemstad
Copy link
Contributor

rerun tests

@cwharris
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 77dc477 into rapidsai:branch-21.12 Nov 11, 2021
@cwharris cwharris mentioned this pull request Nov 11, 2021
Merged
rapids-bot bot pushed a commit that referenced this pull request Nov 11, 2021
@cwharris
Followup to PR 9088 comments (#9659)
36b3344 
These are some minor updates requested for PR #9088 that I forgot to push prior to merging the PR.

Authors:
  - Christopher Harris (https://github.com/cwharris)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Nghia Truong (https://github.com/ttnghia)

URL: #9659
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vuule vuule vuule left review comments

@rgsl888prabhu rgsl888prabhu rgsl888prabhu approved these changes

@mythrocks mythrocks mythrocks approved these changes

@elstehle elstehle elstehle approved these changes

Assignees
No one assigned
Labels
2 - In Progress Currently a work in progress cuIO cuIO issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cwharris @vyasr @vuule @karthikeyann @jrhemstad @elstehle @mythrocks @rgsl888prabhu