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

[REVIEW] cudf strings column #2811

Merged
merged 73 commits into from
Oct 7, 2019
Merged

[REVIEW] cudf strings column #2811

merged 73 commits into from
Oct 7, 2019

Conversation

davidwendt
Copy link
Contributor

This is the cudf strings column work based on #2207
The intention is to convert NVStrings C++ code over to use and create cudf::column classes.
This will become the basis for the cudf python interface for strings as well.

@davidwendt davidwendt requested review from a team as code owners September 16, 2019 19:15
jrhemstad
jrhemstad requested changes Sep 16, 2019
View reviewed changes
cpp/src/column/column_device_view.cu Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_factories.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_factories.hpp Outdated Show resolved Hide resolved
cpp/src/strings/strings_column_factories.cu Outdated Show resolved Hide resolved
cpp/src/strings/strings_column_factories.cu Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/src/strings/strings_column_handler.cu Outdated Show resolved Hide resolved
cpp/src/strings/strings_column_handler.cu Outdated Show resolved Hide resolved
@davidwendt davidwendt mentioned this pull request Sep 17, 2019
Merged
jrhemstad
jrhemstad requested changes Sep 19, 2019
View reviewed changes
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/strings_column_handler.hpp Outdated Show resolved Hide resolved
@kkraus14 kkraus14 added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python) labels Sep 20, 2019
@kkraus14
Copy link
Collaborator

Assigned to 0.10 just so it's in a project board, but feel free to move to 0.11 or wherever else.

@davidwendt
Copy link
Contributor Author

rerun tests

@mike-wendt
Copy link
Contributor

rerun tests

@mike-wendt
Copy link
Contributor

Had to redo tests do to a back hypothesis package that has now been reverted

@davidwendt davidwendt mentioned this pull request Oct 4, 2019
Merged
jrhemstad
jrhemstad requested changes Oct 4, 2019
View reviewed changes
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

One last safety check, and then it looks good to me. Great work!

cpp/src/strings/strings_column_factories.cu Show resolved Hide resolved
devavret
devavret approved these changes Oct 4, 2019
View reviewed changes
Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

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

Sorry I was late to review. Looks good. Couldn't find any problems in the logic. Only thing that looks a bit iffy is the logic to create device view for columns and tables in case of hierarchical columns but I can't think of a better way.

cpp/include/cudf/strings/string_view.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/strings/string_view.inl Outdated Show resolved Hide resolved
cpp/src/column/column_device_view.cu Outdated Show resolved Hide resolved
jrhemstad
jrhemstad requested changes Oct 4, 2019
View reviewed changes
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

I forgot to check this before.

cpp/tests/strings/factories_test.cu Outdated Show resolved Hide resolved
@jrhemstad jrhemstad requested a review from harrism October 7, 2019 16:14
@jrhemstad
Copy link
Contributor

@harrism waiting for your approval to merge.

@davidwendt davidwendt mentioned this pull request Oct 7, 2019
Merged
@harrism harrism merged commit 6693374 into rapidsai:branch-0.10 Oct 7, 2019
@jrhemstad jrhemstad mentioned this pull request Oct 8, 2019
Closed
@davidwendt davidwendt deleted the fea-ext-column-redesign branch October 10, 2019 21:01
@davidwendt davidwendt mentioned this pull request Oct 16, 2019
Merged
@sameerz sameerz mentioned this pull request Dec 20, 2019
Closed
@bdice bdice mentioned this pull request Aug 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devavret devavret devavret approved these changes

@jrhemstad jrhemstad jrhemstad approved these changes

@harrism harrism harrism approved these changes

@karthikeyann karthikeyann Awaiting requested review from karthikeyann

Assignees

@davidwendt davidwendt

Labels
3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@davidwendt @kkraus14 @jrhemstad @harrism @mike-wendt @devavret