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

Merged
merged 36 commits into from
Jan 7, 2025
Merged

Add XXHash_32 hasher #17533

merged 36 commits into from
Jan 7, 2025

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
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
@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.

wence-
wence- approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Approving python changes. Do we have a tracking issue for removal of the hash_combine step when only hashing a single column?

@PointKernel
Copy link
Member Author

Approving python changes. Do we have a tracking issue for removal of the hash_combine step when only hashing a single column?

Good point.

Opened #17687 to track this. Thank you @wence-

@PointKernel
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit caf97ef into rapidsai:branch-25.02 Jan 7, 2025
109 checks passed
@PointKernel PointKernel deleted the add-xxhash32 branch January 7, 2025 17:46
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

@wence- wence- wence- 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

@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
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PointKernel @bdice @davidwendt @wence-