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

Use merge sort for struct types in non-key columns #4459

Merged
merged 4 commits into from
Jan 11, 2022
Merged

Use merge sort for struct types in non-key columns #4459

merged 4 commits into from
Jan 11, 2022

Conversation

firestarman
Copy link
Collaborator

@firestarman firestarman commented Jan 5, 2022 •
edited
Loading

This partially addresses the issue #2252, since cudf merge only supports structs now for nested types.

Signed-off-by: Firestarman firestarmanllc@gmail.com

@firestarman
Support struct type in sort merge
d0988f3 
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
@firestarman
Copy link
Collaborator Author

Marking as draft because I am doing some bencmarks.

@firestarman
Copy link
Collaborator Author

build

@firestarman
comment update
b23c568 
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
@firestarman
Copy link
Collaborator Author

build

@sameerz sameerz added the task Work required that improves the product but is not user facing label Jan 5, 2022
@firestarman
Fix the test error
e3a5ca3 
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
@firestarman
Copy link
Collaborator Author

build

@firestarman
Copy link
Collaborator Author

ready for review

@firestarman firestarman marked this pull request as ready for review January 7, 2022 02:33
jlowe
jlowe reviewed Jan 7, 2022
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.

Since this is adding new support for a type, are there tests to verify the new functionality is working (and continues to work going forward)?

@firestarman
Copy link
Collaborator Author

firestarman commented Jan 10, 2022 •
edited
Loading

Since this is adding new support for a type, are there tests to verify the new functionality is working (and continues to work going forward)?

Sorry for the confusion. This only changes the code path which the processing of merging batches will run into inside this GpuSorter when there is a struct column in the non-key columns, not add a new type support. So the original tests will cover this change.
Updated the title.

@firestarman firestarman changed the title Support struct type in mergeSort Change the path for struct type in mergeSort Jan 10, 2022
@firestarman firestarman changed the title Change the path for struct type in mergeSort Change the path for struct types in mergeSort Jan 10, 2022
@firestarman firestarman changed the title Change the path for struct types in mergeSort Use merge sort for struct types in non-key columns Jan 10, 2022
@firestarman
Address the comments
398c674 
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
@firestarman
Copy link
Collaborator Author

build

@firestarman firestarman merged commit adbbfcd into NVIDIA:branch-22.02 Jan 11, 2022
@firestarman firestarman deleted the merge-struct branch January 11, 2022 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlowe jlowe jlowe approved these changes

@revans2 revans2 Awaiting requested review from revans2

Assignees
No one assigned
Labels
task Work required that improves the product but is not user facing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@firestarman @jlowe @sameerz