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

Cleanup iterator.cuh and add fixed point support for scalar_optional_accessor #10999

Merged
merged 6 commits into from
Jun 1, 2022

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented May 27, 2022

This PR does the following:

  • Cleanup iterator.cuh, enforcing const qualifier for accessors' operator().
  • Remove doxygen for the operator() functions, as they seem trivial: the struct iterators already state clearly their purpose.
  • Make all the operator() functions __device__ callable only: there is no need to call them (and they are never called) from host code.
  • Add template <bool safe = false> for make_validity_iterator(scalar const&) overload, to make it having identical signature as the make_validity_iterator(column_device_view const&) overload.
  • Add fixed point support for scalar_optional_accessor.

This unblocks nested types support in lists::contains (#10548), which needs fixed point support for scalar_optional_accessor.

@ttnghia ttnghia added 3 - Ready for Review Ready for review by team libcudf blocker libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 27, 2022
@ttnghia ttnghia requested a review from a team as a code owner May 27, 2022 18:06
@ttnghia ttnghia self-assigned this May 27, 2022
@ttnghia ttnghia requested review from vyasr and bdice May 27, 2022 18:06
@codecov

This comment was marked as off-topic.

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.

Why return type const qualifier is considered redundant ?
return type is marked const so that the user does not try to modify the dereferenced value.
Look at https://godbolt.org/z/n5MjTxqrf

@ttnghia
Copy link
Contributor Author

ttnghia commented May 28, 2022

Why return type const qualifier is considered redundant ? return type is marked const so that the user does not try to modify the dereferenced value. Look at https://godbolt.org/z/n5MjTxqrf

Ah I see the purpose:

b(2) = std::pair<int, bool>(1, true); // Compiler error.

Typically we don't need that, but here we have the iterators naming accessor which imply "read and write" but we only provide reading access.

@ttnghia ttnghia requested a review from karthikeyann May 28, 2022 20:47
@karthikeyann
Copy link
Contributor

rerun tests

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.

Thanks, this looks good to me!

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.

LGTM 👍

@ttnghia
Copy link
Contributor Author

ttnghia commented Jun 1, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit c76cb9e into rapidsai:branch-22.08 Jun 1, 2022
@ttnghia ttnghia deleted the refactor_iterator_cuh branch June 1, 2022 13:22
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.

4 participants