Skip to content

Refactor ORC reader#13396

Merged
rapids-bot[bot] merged 53 commits intorapidsai:branch-23.08from
ttnghia:refactor_orc_reader
Jun 28, 2023
Merged

Refactor ORC reader#13396
rapids-bot[bot] merged 53 commits intorapidsai:branch-23.08from
ttnghia:refactor_orc_reader

Conversation

@ttnghia
Copy link
Copy Markdown
Contributor

@ttnghia ttnghia commented May 19, 2023

This rewrites the implementation ORC reader, preparing for the next changes in ORC chunked reader:

  • Remove the stream parameter from the read() API. Instead, stream and mr are stored upon reader construction.
  • Remove the inclusion of unused headers.
  • Remove unused variable(s).
  • Rename and re-order member variables.
  • Move (almost) all member functions into free functions. This reduces header dependency between TUs for any future changes.
  • Re-order the functions by their called order.

There is not any new functionality was added or removed.

@ttnghia ttnghia added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 19, 2023
@ttnghia ttnghia self-assigned this May 19, 2023
@ttnghia ttnghia force-pushed the refactor_orc_reader branch from ae2ba31 to f38a56c Compare May 30, 2023 22:50
@ttnghia ttnghia marked this pull request as ready for review June 7, 2023 21:05
@ttnghia ttnghia requested a review from a team as a code owner June 7, 2023 21:05
@ttnghia ttnghia marked this pull request as draft June 7, 2023 21:05
@ttnghia ttnghia added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jun 14, 2023
@ttnghia ttnghia requested review from karthikeyann and vuule June 14, 2023 04:12
Copy link
Copy Markdown
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.

I'm glad this PR is kept simple. Even this much is hard to review because of code shuffle. It should make future PRs much easier on the eye :)

@ttnghia ttnghia requested a review from vuule June 26, 2023 18:49
stream);

// Value for checking whether we decompress successfully.
// It doesn't need to be atomic as there is no race condition: we only write `false` if needed.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏

Copy link
Copy Markdown
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.

LGTM! It certainly reads much better now.

@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 Jun 28, 2023
@ttnghia
Copy link
Copy Markdown
Contributor Author

ttnghia commented Jun 28, 2023

/merge

@rapids-bot rapids-bot bot merged commit 5c615cc into rapidsai:branch-23.08 Jun 28, 2023
@ttnghia ttnghia deleted the refactor_orc_reader branch June 29, 2023 01:04
rapids-bot bot pushed a commit that referenced this pull request Jun 29, 2023
…kernel (#13643)

Fixes memcheck regression in ORC reader uncompressed logic. The regression was introduced in #13396.
The original fix is copied from #13586 

The error was caught by the nightly build here: https://github.com/rapidsai/cudf/actions/runs/5409203449/jobs/9829059618

Authors:
  - David Wendt (https://github.com/davidwendt)

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

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

Labels

5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants