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

[FEA] Enable dynamic "has_nulls" for row operators #9580

Closed
jrhemstad opened this issue Nov 2, 2021 · 3 comments
Closed

[FEA] Enable dynamic "has_nulls" for row operators #9580

jrhemstad opened this issue Nov 2, 2021 · 3 comments
Assignees
Labels
feature request New feature or request improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code.

Comments

@jrhemstad
Copy link
Contributor

Is your feature request related to a problem? Please describe.

The row_lexicographic_comparator and row_equality_comparator have a has_nulls non-type template parameter to allow optimizing away the more expensive null handling code.

While this is nice in cases where the null logic actually impacts performance, there are times where it doesn't. In the cases where it doesn't matter for performance, it needlessly increases compile time by having two versions of a kernel (has_nulls==true and has_nulls==false).

We should make it so this information can be dynamic in addition to static.

Describe the solution you'd like

Similar to optional_iterator change the has_nulls to have three options:

row_equality_comparator<HAS_NULLS::YES>{...}
row_equality_comparator<HAS_NULLS::NO>{...}
row_equality_comparator<HAS_NULLS::DYNAMIC>{..., has_nulls}; // runtime value for has_nulls

The implementation should be specialized to ignore the has_nulls dynamic value for the two static cases.

@jrhemstad jrhemstad added feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. tech debt improvement Improvement / enhancement to an existing function labels Nov 2, 2021
@davidwendt
Copy link
Contributor

I had already started looking at an approach for this awhile back. Summary of my findings are in #6952

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

rapids-bot bot pushed a commit that referenced this issue Dec 6, 2021
Closes #6952 

This PR allows the `has_nulls` template parameter for row operators to be used a runtime parameter in places where the null-handling logic has little to no affect on runtime performance. 
This can improve compile time as described in #6952.

This will also close #9152 and #9580

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

Approvers:
  - Jake Hemstad (https://github.com/jrhemstad)
  - Nghia Truong (https://github.com/ttnghia)
  - Conor Hoekstra (https://github.com/codereport)

URL: #9623
@davidwendt
Copy link
Contributor

Closed by #9623

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

No branches or pull requests

2 participants