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 row hasher with nested column support #10641

Merged
merged 182 commits into from
Apr 29, 2022
Merged

Add row hasher with nested column support #10641

merged 182 commits into from
Apr 29, 2022

Conversation

devavret
Copy link
Contributor

Contributes to #10186

devavret added 30 commits August 26, 2021 19:28
@devavret
First commit
933c974 
Many iterations already happened. I just realized late that I should commit
@devavret
testing and profiling deep single hierarchy struct
a1636e5
@devavret
Merge branch 'branch-22.02' into struct-row-comp
d59f54c
@devavret
Merge branch 'branch-22.02' into struct-row-comp
765dd8d
@devavret
Make the sandboxed test compile again
3d21daf
@devavret
Update my row_comparator with nullate
9f32e6b
@devavret
Merge branch 'branch-22.02' into struct-row-comp
53d3c90
@devavret
Basic verticalization utility and experimental namespace
022e2a4
@devavret
clean up most of row operators that I didn't change.
7fef643
@devavret
Sliced column test
930d8de
@devavret
column order and null precendence support
0ecc4f8
@devavret
Manually managed stack
ff36d2d
@devavret
New depth based method to avoid superimpose nulls
cd0f938 
sliced no longer works
@devavret
Put sort2 impl in separate TU
7b8e060
@devavret
Merge branch 'branch-22.04' into struct-row-comp
25eb237
@devavret
Basic working list == comp
c8e527e
@devavret
Merge branch 'branch-22.04' into list-row-eq
eb87ed7
@devavret
deeper list test
cc1584d
@devavret
benchmark list ==
925481a
@devavret
small cleanups
b2b41c7
@devavret
List hash working with drop duplicates
8aaf6f3
@devavret
Merge branch 'branch-22.04' into struct-row-comp
d2937cf
@devavret
Move verticalization code to row_comparator.cpp
d55c9c7
@devavret
Merge branch 'struct-row-comp' into list-row-eq
b7cdfe0
@devavret
Use regular type dispatcher with new id type map
8309151
@devavret
Early return from unequal leaf elements
8717b9c
@devavret
Combined struct and list equality operator
21df6cf 
New verticalization code that includes list all the way up, skipping structs that were included
@devavret
Add null testing to list level also
fa35461
@devavret
Owning row lex operator
3bd749e
@devavret
merge fixes
613d664
@vyasr
Copy link
Contributor

vyasr commented Apr 26, 2022

@bdice @jrhemstad I've addressed the remaining comments that I either viewed as blockers or as simple enough to be easily fixed without expanding this PR's scope. I opened two issue for follow-ups regarding 1) improved testing and 2) further performance profiling, both of which I think can be done after this PR is merged. I have held off approving myself so that this PR is not accidentally merged before reviewers are happy, but I think this should be good to go once you two are happy. I'm happy to address future change requests on this PR while Devavret is out.

bdice
bdice reviewed Apr 26, 2022
View reviewed changes
Copy link
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.

Feedback attached. I'll discuss with @vyasr and one of us will resolve these comments.

cpp/src/hash/hashing.cu Outdated Show resolved Hide resolved
cpp/src/hash/murmur_hash.cu Outdated Show resolved Hide resolved
cpp/src/hash/murmur_hash.cu Show resolved Hide resolved
cpp/src/table/row_operators.cu Outdated Show resolved Hide resolved
cpp/tests/hashing/hash_test.cpp Outdated Show resolved Hide resolved
bdice
bdice approved these changes Apr 28, 2022
View reviewed changes
Copy link
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.

I'm happy with this PR. I have two very small comments/questions but we should move forward and get this merged to unblock other work.

cpp/include/cudf/table/experimental/row_operators.cuh Show resolved Hide resolved
cpp/include/cudf/table/experimental/row_operators.cuh Show resolved Hide resolved
@bdice bdice dismissed vyasr’s stale review April 28, 2022 21:33

Dismissing stale review.

@bdice
Copy link
Contributor

bdice commented Apr 28, 2022

I discussed this with @vyasr earlier today. I think we're ready but I'll let @vyasr merge in case he had any final thoughts. I think we're all in agreement that this is really great work, thank you very much to @devavret! 🔥

@vyasr
Copy link
Contributor

vyasr commented Apr 29, 2022

I've got one question but I think we can merge and then discuss it. This looks ready to go to me!

@vyasr
Copy link
Contributor

vyasr commented Apr 29, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 3c4e72e into rapidsai:branch-22.06 Apr 29, 2022
@devavret
Copy link
Contributor Author

I've got one question but I think we can merge and then discuss it. This looks ready to go to me!

Ask away!

@bdice bdice mentioned this pull request May 9, 2022
Closed
@devavret devavret mentioned this pull request May 16, 2022
Merged
@GregoryKimball GregoryKimball mentioned this pull request Jun 30, 2022
Closed
@GregoryKimball GregoryKimball mentioned this pull request Oct 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PointKernel PointKernel PointKernel approved these changes

@bdice bdice bdice approved these changes

@vyasr vyasr vyasr left review comments

@ttnghia ttnghia ttnghia approved these changes

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

@jrhemstad jrhemstad Awaiting requested review from jrhemstad

Assignees

@vyasr vyasr

@devavret devavret

@bdice bdice

Labels
4 - Needs Review Waiting for reviewer to review or respond feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@devavret @vyasr @bdice @ajschmidt8 @ttnghia @PointKernel @jrhemstad