Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add minhash support for MurmurHash3_x64_128 #13796
Add minhash support for MurmurHash3_x64_128 #13796
Changes from 9 commits
6004d35
f51287d
acec8a8
9775001
6ef09ba
ef22254
74a11ea
35ce135
287e3d9
72b66c2
2389901
0ecc9c6
180c619
706b1e4
fd2f505
b335f1f
07483a5
6a5226f
e2fd975
df38760
3780d96
9052e6f
23d88ca
6690e27
b5fde2e
638a65d
b347eca
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why switch to hardcoding the value here instead of using the constant?
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.
Because the type may be different. I'd rather be clear that the default seed is actually 0 and would not want to change that if the rest of libcudf decided on a different default. Hopefully that is ok.
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.
Same question as before, why the different seed choice? Is it because of the potential for unsafe casts depending on the type of
DEFAULT_HASH_SEED
(currently OK since it'suint32_t
)?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, the type is different here and I'd rather the 2 functions be consistent over any need to use the constant def.