-
Notifications
You must be signed in to change notification settings - Fork 915
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
Update distinct/unique_count
to experimental::row
hasher/comparator
#12776
Update distinct/unique_count
to experimental::row
hasher/comparator
#12776
Conversation
Co-authored-by: Nghia Truong <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of minor questions.
Co-authored-by: Vyas Ramasubramani <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are benchmarks impacted by this PR?
…action-row-hasher
There are no benchmarks for these algorithms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor question, otherwise LGTM. Is the plan to keep moving forward with these for now and investigate compile times when we can?
rmm::exec_policy(stream), | ||
thrust::counting_iterator<cudf::size_type>(0), | ||
thrust::counting_iterator<cudf::size_type>(keys.num_rows()), | ||
[comp] __device__(cudf::size_type i) { return (i == 0 or not comp(i, i - 1)); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that we can't capture the comparator by reference because it's a host object that needs to be copied to device for the lambda?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct
@vyasr and I followed up offline on his compile time question. |
/merge |
This PR is a part of #11844
Checklist
Compilation Times
distinct_count.cu
This PR: 3m8.392s
main: 1m37.576s
unique_count.cu
This PR: 13m8.858s
main: 8m21.900s