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

Refactor io::orc::ProtobufWriter #12877

Merged
merged 16 commits into from
Mar 14, 2023
Merged

Refactor io::orc::ProtobufWriter #12877

merged 16 commits into from
Mar 14, 2023

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Mar 2, 2023 •
edited
Loading

This refactors the class cudf::io::orc::ProtobufWriter, making it independent from the ORC writer buffer. From now, a new instance of ProtobufWriter will work on its own buffer. That avoids touching the ORC writer's internal states.

The PR is part of solution for #12792.

ttnghia added 2 commits March 1, 2023 11:22
@ttnghia
Move const position
9422fd5 
Signed-off-by: Nghia Truong <[email protected]>
@ttnghia
Get rid of the internal buffer_ state
33d541e 
Signed-off-by: Nghia Truong <[email protected]>
@ttnghia ttnghia added feature request New feature or request 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS non-breaking Non-breaking change Reliability labels Mar 2, 2023
@ttnghia ttnghia self-assigned this Mar 2, 2023
@ttnghia ttnghia marked this pull request as ready for review March 6, 2023 18:14
@ttnghia ttnghia requested a review from a team as a code owner March 6, 2023 18:14
@ttnghia ttnghia added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Mar 6, 2023
@ttnghia ttnghia requested a review from vuule March 7, 2023 05:04
vuule
vuule requested changes Mar 7, 2023
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.

Is this based on my issue #7015 or a convergent idea? :)

Some comments about implementation details.

cpp/src/io/orc/orc.hpp Outdated Show resolved Hide resolved
cpp/src/io/orc/orc.hpp Outdated Show resolved Hide resolved
@randerzander
Copy link
Contributor

benchmark bot, please test this PR

@ttnghia ttnghia added the cuIO cuIO issue label Mar 9, 2023
@ttnghia ttnghia requested a review from vuule March 10, 2023 16:41
vuule
vuule reviewed Mar 10, 2023
View reviewed changes
cpp/src/io/orc/orc.hpp Outdated Show resolved Hide resolved
vuule
vuule approved these changes Mar 14, 2023
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.

💯

@DaceT
Copy link

DaceT commented Mar 14, 2023

benchmark bot, please test this PR

nvdbaranec
nvdbaranec reviewed Mar 14, 2023
View reviewed changes
Copy link
Contributor

@nvdbaranec nvdbaranec left a comment

Choose a reason for hiding this comment

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

Looks ok. One comment on a possible clarifying constructor.

cpp/src/io/orc/writer_impl.cu Outdated Show resolved Hide resolved
@ttnghia ttnghia requested a review from nvdbaranec March 14, 2023 21:33
@vuule vuule added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Mar 14, 2023
@ttnghia
Copy link
Contributor Author

ttnghia commented Mar 14, 2023

/merge

@rapids-bot rapids-bot bot merged commit 745d35e into rapidsai:branch-23.04 Mar 14, 2023
@ttnghia ttnghia deleted the refactor_protocol_buffer branch March 14, 2023 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvdbaranec nvdbaranec nvdbaranec approved these changes

@vuule vuule vuule approved these changes

@robertmaynard robertmaynard Awaiting requested review from robertmaynard robertmaynard is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees

@ttnghia ttnghia

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge cuIO cuIO issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ttnghia @randerzander @DaceT @vuule @nvdbaranec