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.

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

Refactor lists::contains #11019

Merged
merged 218 commits into from
Jun 23, 2022
Merged

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Jun 1, 2022

This is just a not-very-simple refactor to lists::contains:

  • Remove some wrong lines in doxygen of lists/contains.hpp, and rewrite doxygen there a little bit.
  • Add more comments to the code.
  • Reduce the number of code paths of the template struct functors.
  • Rename some/many variables, and reorganize code to make it cleaner.

No new feature is added in this PR, just modifying the existing functions and moving things around.

This PR is extracted from the bigger PR for easier review. The original PR is #10548 for supporting nested type in lists::contains. As such, this blocks it.

@ttnghia ttnghia requested review from jrhemstad and removed request for codereport and vyasr June 16, 2022 04:55
rapids-bot bot pushed a commit that referenced this pull request Jun 16, 2022
This PR updates `clamp.cu` to take templated iterator types by value. This aligns the style with the rest of libcudf. See also: #11019 (comment)

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - David Wendt (https://github.com/davidwendt)
  - Mark Harris (https://github.com/harrism)

URL: #11084
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Nice. This has improved a lot with review.

@ttnghia
Copy link
Contributor Author

ttnghia commented Jun 22, 2022

Thanks all for reviewing. Yeah, the implementation has been improved significantly with your help ❤️

Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

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

Minor comments. Rest look good. 👍

cpp/src/lists/contains.cu Outdated Show resolved Hide resolved
cpp/src/lists/contains.cu Show resolved Hide resolved
@ttnghia
Copy link
Contributor Author

ttnghia commented Jun 23, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0e08022 into rapidsai:branch-22.08 Jun 23, 2022
@ttnghia ttnghia deleted the refactor_lists_contains branch June 23, 2022 15:31
rapids-bot bot pushed a commit that referenced this pull request Aug 22, 2022
This extends the `lists::contains` API to support nested types (lists + structs) with arbitrarily nested levels. As such, `lists::contains` will work with literally any type of input data.

In addition, the related implementation has been significantly refactored to facilitate adding new implementation.

Closes #8958.
Depends on:
 * #10730
 * #10883
 * #10999
 * #11019
 * #11037

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

Approvers:
  - MithunR (https://github.com/mythrocks)
  - Bradley Dice (https://github.com/bdice)

URL: #10548
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants