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

Add XXHash_32 hasher #17533

Open
wants to merge 36 commits into
base: branch-25.02
Choose a base branch
from
Open

Add XXHash_32 hasher #17533

wants to merge 36 commits into from
+473 −35

Conversation

PointKernel
Copy link
Member

@PointKernel PointKernel commented Dec 5, 2024 •
edited
Loading

Description

Contributes to #17531

This PR introduces the xxhash_32 hasher to libcudf as a preparatory step for evaluating the impact of replacing murmurhash3_x86_32 with xxhash_32 as the default hash.

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 libcudf Affects libcudf (C++/CUDA) code. Performance Performance related issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Dec 5, 2024
@PointKernel PointKernel self-assigned this Dec 5, 2024
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Dec 5, 2024

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@PointKernel
Copy link
Member Author

/ok to test

@PointKernel
Copy link
Member Author

/ok to test

@bdice
Copy link
Contributor

bdice commented Dec 6, 2024

Nice work so far @PointKernel. I was experimenting with the same thing locally and had most of the same changes. 😄
Even if we end up deciding not to go forward with this change, I see a few things that we definitely want to merge (missing includes, and fixes for other fragility in tests/etc. exposed by the change of default).

@PointKernel
Copy link
Member Author

Thanks, @bdice. The failing tests are a bit concerning, but I'll investigate them further.

PointKernel and others added 2 commits December 6, 2024 13:39
@PointKernel
Copy link
Member Author

/ok to test

@davidwendt
Copy link
Contributor

Perhaps plumb this through as a public function as well here?
https://github.com/rapidsai/cudf/blob/branch-25.02/cpp/include/cudf/hashing.hpp

@github-actions github-actions bot added the CMake CMake build issue label Dec 11, 2024
@PointKernel PointKernel changed the title Use XXHash_32 as the default hash Add XXHash_32 hasher Dec 16, 2024
@bdice
Copy link
Contributor

bdice commented Dec 16, 2024

@PointKernel Yes! I support that plan.

bdice
bdice approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We'll need a follow-up PR for the Python side.

edit: we decided to do it here!

@bdice bdice requested a review from a team as a code owner December 16, 2024 21:52
@bdice bdice requested review from wence- and Matt711 December 16, 2024 21:52
@github-actions github-actions bot added Python Affects Python cuDF API. pylibcudf Issues specific to the pylibcudf package labels Dec 16, 2024
@PointKernel PointKernel added the 4 - Needs Review Waiting for reviewer to review or respond label Jan 6, 2025
davidwendt
davidwendt approved these changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

Approving C++ changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

@davidwendt davidwendt davidwendt approved these changes

@harrism harrism Awaiting requested review from harrism harrism is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@pmattione-nvidia pmattione-nvidia Awaiting requested review from pmattione-nvidia pmattione-nvidia is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@wence- wence- Awaiting requested review from wence- wence- is a code owner automatically assigned from rapidsai/cudf-python-codeowners

@Matt711 Matt711 Awaiting requested review from Matt711 Matt711 is a code owner automatically assigned from rapidsai/cudf-python-codeowners

Assignees

@bdice bdice

@PointKernel PointKernel

Labels
3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package Python Affects Python cuDF API.
Projects
cuDF Python
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PointKernel @bdice @davidwendt