-
Notifications
You must be signed in to change notification settings - Fork 915
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.
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
Support nested structs in rank and dense rank #8962
Support nested structs in rank and dense rank #8962
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #8962 +/- ##
===============================================
Coverage ? 10.82%
===============================================
Files ? 115
Lines ? 19166
Branches ? 0
===============================================
Hits ? 2074
Misses ? 17092
Partials ? 0 Continue to review full report at Codecov.
|
Moving back to draft because of parent null inheritance assumption concerns. Will rework around changes to the flattening approach and null checking process. |
52ca8ef
to
b3508ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how this cleaned up and boiled down to basically the same pattern for all of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, it is much cleaner now that you've pushed the null handling down to the comparator. Would it be possible to change generate_ranks
and generate_dense_ranks
into a single internal function that accepts a device lambda for the tabulate
and a predicate for the inclusive_scan_by_key
? I believe that the rest of the code is entirely identical now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look great. Nice to see that the templating made this a negative LOC PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmake approval
@gpucibot merge |
Follow on to #8652 for nested struct support using, partially removing the need for #8683.
This change simplifies the rank algorithm by assuming
superimpose_parent_nulls
has been ran on the struct column. This removes the need for separate logic that ensures we are not comparing elements covered by a parent column's null mask.