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

Move SparkMurmurHash3_32 functor. #11489

Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Aug 8, 2022

Description

This PR moves the SparkMurmurHash3_32 functor from hash_functions.cuh to spark_murmur_hash.cu, the only place where it is used. This is a pure move, with one small exception to avoid compiler warnings about unused members of the hash functor template instantiations for nested types. I refactored the class template to disallow nested types for the hash functor and removed those specializations using CUDF_UNREACHABLE, rather than allowing type dispatching to create template instantiations that have no defined use. (Nested types are being handled by the custom device row hasher in spark_murmur_hash.cu, and require some state information that cannot be easily carried in the functor itself.) I am planning to do further refactoring later, but wanted to separate this "pure move" as much as possible.

Part of #10081.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@bdice bdice added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 8, 2022
@bdice bdice self-assigned this Aug 8, 2022
@bdice bdice requested a review from a team as a code owner August 8, 2022 03:47
@bdice bdice requested review from vyasr, jrhemstad and rwlee and removed request for vyasr August 8, 2022 03:47
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Aug 8, 2022
@bdice
Copy link
Contributor Author

bdice commented Aug 8, 2022

rerun tests

@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@e1a4e03). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.10   #11489   +/-   ##
===============================================
  Coverage                ?   86.47%           
===============================================
  Files                   ?      144           
  Lines                   ?    22852           
  Branches                ?        0           
===============================================
  Hits                    ?    19761           
  Misses                  ?     3091           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@rwlee rwlee left a comment

Choose a reason for hiding this comment

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

+1

@bdice
Copy link
Contributor Author

bdice commented Aug 9, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit fea0bda into rapidsai:branch-22.10 Aug 9, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants