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.

Sign up for GitHub

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

Jump to bottom

Remove HASH_SERIAL_MURMUR3 / serial32BitMurmurHash3 #11296

Closed
bdice opened this issue Jul 18, 2022 · 1 comment · Fixed by #11383
Closed

Remove HASH_SERIAL_MURMUR3 / serial32BitMurmurHash3 #11296

bdice opened this issue Jul 18, 2022 · 1 comment · Fixed by #11383
Assignees
@bdice
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.
Projects
v22.10 Release

Comments

@bdice
Copy link
Contributor

bdice commented Jul 18, 2022 •
edited
Loading

Is your feature request related to a problem? Please describe.
While reimplementing Spark hashing in #11292, I noticed that HASH_SERIAL_MURMUR3 does not appear to be used except in tests. It is not exposed in Python. While it is exposed in the JNI bindings, it is not used by spark-rapids. I discussed this with @rwlee and it seems that this feature was added only for parallel design with the Spark serial hash implementation in #6781. We may not need to keep this feature in the code base.

Describe the solution you'd like
After #11292 is complete, the serial_murmur_hash3_32 template function used for HASH_SERIAL_MURMUR3 won't be needed by HASH_SPARK_MURMUR3. Then evaluate deprecation/removal of the HASH_SERIAL_MURMUR3 feature.
@bdice bdice added feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. tech debt labels Jul 18, 2022
@bdice bdice self-assigned this Jul 18, 2022
@bdice bdice added this to Issue-Needs prioritizing in v22.10 Release via automation Jul 18, 2022
This was referenced Jul 20, 2022
Merged
Closed
@bdice bdice mentioned this issue Jul 28, 2022
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this issue Aug 1, 2022
@bdice
Remove HASH_SERIAL_MURMUR3 / serial32BitMurmurHash3 (#11383)
1d4aa4a 
This PR closes #11296. While implementing Spark list hashing in #11292, I noticed that `HASH_SERIAL_MURMUR3` does not appear to be used except in tests. It is not exposed in Python. While it is exposed in the JNI bindings, it is not used by spark-rapids. I discussed this with @rwlee and it seems that this feature was added only for parallel design with the Spark serial hash implementation in #6781, which is superseded by #11292. We do not need to keep this vestigial feature.

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

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - https://github.com/brandon-b-miller
  - David Wendt (https://github.com/davidwendt)
  - Jason Lowe (https://github.com/jlowe)

URL: #11383
@bdice
Copy link
Contributor Author

bdice commented Aug 8, 2022

Resolved by #11383.

@bdice bdice closed this as completed Aug 8, 2022
v22.10 Release automation moved this from Issue-Needs prioritizing to Done Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bdice bdice

Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.
Projects
No open projects
v22.10 Release
  
Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove HASH_SERIAL_MURMUR3 / serial32BitMurmurHash3 bdice/cudf
1 participant
@bdice