Skip to content

Update MurmurHash3_x86_32 to use the cuco equivalent implementation#17429

Merged
rapids-bot[bot] merged 15 commits intorapidsai:branch-25.02from
PointKernel:cleanup-murmurhash-32
Nov 27, 2024
Merged

Update MurmurHash3_x86_32 to use the cuco equivalent implementation#17429
rapids-bot[bot] merged 15 commits intorapidsai:branch-25.02from
PointKernel:cleanup-murmurhash-32

Conversation

@PointKernel
Copy link
Copy Markdown
Member

@PointKernel PointKernel commented Nov 23, 2024

Description

This PR modifies MurmurHash3_x86_32 to utilize the cuco equivalent implementation, eliminating duplication. Additionally, it resolves a minor issue in xxhash_64 where the seed was not correctly passed to the underlying implementation.

Checklist

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

@PointKernel PointKernel added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 23, 2024
@PointKernel PointKernel self-assigned this Nov 23, 2024
@github-actions github-actions bot added the CMake CMake build issue label Nov 24, 2024
@PointKernel PointKernel force-pushed the cleanup-murmurhash-32 branch from fbe4c7f to 278eb1f Compare November 25, 2024 05:20
@PointKernel PointKernel marked this pull request as ready for review November 25, 2024 18:53
@PointKernel PointKernel requested review from a team as code owners November 25, 2024 18:53
@PointKernel PointKernel requested review from bdice and lamarrr November 25, 2024 18:53
@PointKernel PointKernel added 3 - Ready for Review Ready for review by team cuco cuCollections related issue and removed 2 - In Progress Currently a work in progress labels Nov 25, 2024
Copy link
Copy Markdown
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

This is fine, though I had to rethink my own views on how the hashers work. Thanks @PointKernel for explaining the pitfalls in my expectations.

@PointKernel
Copy link
Copy Markdown
Member Author

@bdice @lamarrr Thank you both for the review! I’ve addressed all your comments in the PR and will open a follow-up PR to apply the same improvement to xxhash, will request your review there as well.

@PointKernel
Copy link
Copy Markdown
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 9db132a into rapidsai:branch-25.02 Nov 27, 2024
@PointKernel PointKernel deleted the cleanup-murmurhash-32 branch November 27, 2024 19:29
rapids-bot bot pushed a commit that referenced this pull request Nov 28, 2024
Incorporate the review suggestions from #17429 into xxhash_64 to clean up the code.

Authors:
  - Yunsong Wang (https://github.com/PointKernel)
  - Muhammad Haseeb (https://github.com/mhaseeb123)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Muhammad Haseeb (https://github.com/mhaseeb123)
  - Basit Ayantunde (https://github.com/lamarrr)

URL: #17455
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 CMake CMake build issue cuco cuCollections related issue 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.

4 participants