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_avro by removing unnecessary writer/impl classes #9090

Merged
merged 23 commits into from
Aug 31, 2021
Merged

Simplify read_avro by removing unnecessary writer/impl classes #9090

merged 23 commits into from
Aug 31, 2021

Conversation

cwharris
Copy link
Contributor

Depends on #9040

cwharris added 13 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
remove unncessary avro reader class
9107f24
@cwharris
replace avro::reader_impl mr member with local variable
f15622e
@cwharris
replace avro::reader_impl source member with local variable
4cf9f32
@cwharris
remove unneccessary avro::reader_impl _columns member
ea2c219
@cwharris
replace avro::reader_impl metadata member with local variable
2c16c19
@cwharris
delete unncessarry avro::reader_impl class
4188c2b
@cwharris
read_avro: replace explicit cudamemcpyasync with uvector factor calls
cdbdb5e
@github-actions github-actions bot added Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Aug 21, 2021
@cwharris cwharris added cuIO cuIO issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change tech debt labels Aug 21, 2021
@codecov
Copy link

codecov bot commented Aug 21, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@a153493). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #9090   +/-   ##
===============================================
  Coverage                ?   10.76%           
===============================================
  Files                   ?      114           
  Lines                   ?    19086           
  Branches                ?        0           
===============================================
  Hits                    ?     2055           
  Misses                  ?    17031           
  Partials                ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a153493...98ab9c9. Read the comment docs.

@github-actions github-actions bot removed the Python Affects Python cuDF API. label Aug 24, 2021
@cwharris cwharris marked this pull request as ready for review August 24, 2021 19:37
@cwharris cwharris requested a review from a team as a code owner August 24, 2021 19:37
ttnghia
ttnghia requested changes Aug 25, 2021
View reviewed changes
cpp/src/io/avro/reader_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/avro/reader_impl.cu Outdated Show resolved Hide resolved
harrism
harrism reviewed Aug 25, 2021
View reviewed changes
cpp/include/cudf/io/detail/avro.hpp Show resolved Hide resolved
cpp/src/io/avro/reader_impl.cu Outdated Show resolved Hide resolved
@cwharris
Copy link
Contributor Author

@ttnghia @harrism This PR contains the bare minimum changes to remove the reader and impl classes for avro. That change reveals a lot of other improvements we can make to the code, which is why I decided to do it, not just for avro but for json and csv as well. JSON and CSV are much more complicated change sets than this one. For those, I think it made sense to restrict the changes to the bare minimum.

After reading your comments, however, it seems that this PR is simple enough that it can still stand some more improvements without becoming overly complicated. I'll take another pass at general improvements to the code. Thanks!

@cwharris
Copy link
Contributor Author

I did another pass, enforcing const-ness where possible, and doing general cleanup.

@cwharris cwharris requested review from ttnghia and harrism August 25, 2021 06:09
ttnghia
ttnghia reviewed Aug 26, 2021
View reviewed changes
Copy link
Contributor

@ttnghia ttnghia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just some more nit about const positions. Hope that I don't miss any 😃

cpp/src/io/avro/avro_gpu.cu Outdated Show resolved Hide resolved
cpp/src/io/avro/avro_gpu.cu Outdated Show resolved Hide resolved
cpp/src/io/avro/avro_gpu.cu Outdated Show resolved Hide resolved
cpp/src/io/avro/avro_gpu.h Outdated Show resolved Hide resolved
cpp/src/io/avro/reader_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/avro/reader_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/avro/reader_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/avro/reader_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/avro/reader_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/avro/reader_impl.cu Outdated Show resolved Hide resolved
@cwharris cwharris requested a review from ttnghia August 26, 2021 03:10
@cwharris
Copy link
Contributor Author

@ttnghia while I prefer east const, it is not enforced by style checks.

ttnghia
ttnghia reviewed Aug 26, 2021
View reviewed changes
cpp/src/io/avro/avro_gpu.h Outdated Show resolved Hide resolved
@cwharris cwharris requested a review from ttnghia August 26, 2021 03:26
@cwharris
Copy link
Contributor Author

rerun tests

@cwharris cwharris requested a review from vuule August 27, 2021 17:57
@cwharris
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4ad09aa into rapidsai:branch-21.10 Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ttnghia ttnghia ttnghia approved these changes

@karthikeyann karthikeyann karthikeyann approved these changes

@harrism harrism Awaiting requested review from harrism

@vuule vuule Awaiting requested review from vuule

Assignees
No one assigned
Labels
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.
4 participants
@cwharris @harrism @karthikeyann @ttnghia