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 xxhash64 support #1248

Merged
merged 27 commits into from
Jul 12, 2023
Merged

Add xxhash64 support #1248

merged 27 commits into from
Jul 12, 2023

Conversation

nvdbaranec
Copy link
Collaborator

@nvdbaranec nvdbaranec commented Jul 1, 2023 •
edited
Loading

Adds support for xxhash64. Includes JNI bindings, cpp tests and java tests.

Does not currently support structs or lists, but those should be relatively easy to add if desired.

Dependent on #1246

@nvdbaranec nvdbaranec added the enhancement New feature or request label Jul 1, 2023
@nvdbaranec nvdbaranec requested review from jlowe and rwlee July 1, 2023 23:05
jlowe
jlowe reviewed Jul 3, 2023
View reviewed changes
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Marking as draft since this depends on #1244 and #1246. This also needs a signoff commit per the contribution guidelines.

src/test/java/com/nvidia/spark/rapids/jni/HashTest.java Outdated Show resolved Hide resolved
@jlowe jlowe marked this pull request as draft July 3, 2023 15:35
jlowe
jlowe reviewed Jul 3, 2023
View reviewed changes
src/main/java/com/nvidia/spark/rapids/jni/Hash.java Outdated Show resolved Hide resolved
src/main/cpp/src/HashJni.cpp Outdated Show resolved Hide resolved
src/main/cpp/src/HashJni.cpp Show resolved Hide resolved
src/main/cpp/src/HashJni.cpp Outdated Show resolved Hide resolved
src/main/cpp/src/HashJni.cpp Outdated Show resolved Hide resolved
jlowe
jlowe reviewed Jul 5, 2023
View reviewed changes
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

lgtm, just need to get the dependencies merged first.

@nvdbaranec nvdbaranec marked this pull request as ready for review July 7, 2023 21:57
@nvdbaranec nvdbaranec requested a review from jlowe July 7, 2023 21:57
jlowe
jlowe requested changes Jul 10, 2023
View reviewed changes
pom.xml Outdated Show resolved Hide resolved
thirdparty/cudf Outdated Show resolved Hide resolved
@nvdbaranec nvdbaranec requested a review from jlowe July 10, 2023 16:40
jlowe
jlowe previously approved these changes Jul 10, 2023
View reviewed changes
@jlowe
Copy link
Member

jlowe commented Jul 10, 2023

build

hyperbolic2346
hyperbolic2346 requested changes Jul 11, 2023
View reviewed changes
Copy link
Collaborator

@hyperbolic2346 hyperbolic2346 left a comment

Choose a reason for hiding this comment

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

Good work on this. I appreciate the tests added for murmur as well. A few nits and questions.

src/main/cpp/src/xxhash64.cu Outdated Show resolved Hide resolved
src/main/cpp/src/xxhash64.cu Outdated Show resolved Hide resolved
src/main/cpp/src/xxhash64.cu Outdated Show resolved Hide resolved
src/main/cpp/src/xxhash64.cu Outdated Show resolved Hide resolved
src/test/java/com/nvidia/spark/rapids/jni/HashTest.java Show resolved Hide resolved
@jlowe
Copy link
Member

jlowe commented Jul 11, 2023

build

@nvdbaranec nvdbaranec merged commit c77ab9d into NVIDIA:branch-23.08 Jul 12, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwlee rwlee rwlee left review comments

@jlowe jlowe jlowe approved these changes

@hyperbolic2346 hyperbolic2346 hyperbolic2346 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nvdbaranec @jlowe @hyperbolic2346 @rwlee