-
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
Use cuco::static_set in the hash-based groupby #14813
Merged
rapids-bot
merged 27 commits into
rapidsai:branch-24.04
from
PointKernel:cuco-set-groupby
Feb 29, 2024
Merged
Changes from 22 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
11b8126
Rewrite hash groupby with hash set
PointKernel 166ed49
Formatting
PointKernel 07313fe
Merge remote-tracking branch 'upstream/branch-24.04' into cuco-set-gr…
PointKernel b1db243
Minor cleanups
PointKernel ed8502d
Merge remote-tracking branch 'upstream/branch-24.04' into cuco-set-gr…
PointKernel ca6829d
Update cuco code
PointKernel 0c10a0b
Add CUCO_CUDF_SIZE_TYPE_SENTINEL
PointKernel 2470c68
Header cleanups
PointKernel 7da8c55
Update docs
PointKernel 7dd59a6
Minor doc updates
PointKernel 3cbdb7c
Add peak memory usage metrics to groupby NV bencmarks
PointKernel 82aa0ce
Revert some benchmark changes
PointKernel 230928a
Merge remote-tracking branch 'upstream/branch-24.04' into cuco-set-gr…
PointKernel 2259455
Merge remote-tracking branch 'upstream/branch-24.04' into cuco-set-gr…
PointKernel 4193c75
Fix pytests
PointKernel 9645865
Merge remote-tracking branch 'upstream/branch-24.04' into cuco-set-gr…
PointKernel 574f628
Renaming
PointKernel 75a8e64
Fix several docstring tests
PointKernel 85a47db
Make value_counts docstring test deterministic
PointKernel 241aca0
Merge remote-tracking branch 'upstream/branch-24.04' into cuco-set-gr…
PointKernel dbd9e6b
Merge branch 'branch-24.04' into cuco-set-groupby
PointKernel 0af1d13
Merge branch 'branch-24.04' into cuco-set-groupby
PointKernel f79f1d6
Update docs
PointKernel 8263b4f
Merge branch 'branch-24.04' into cuco-set-groupby
PointKernel 56a2229
Add TODO reminder for future performance tuning
PointKernel 8bade44
Merge remote-tracking branch 'origin/cuco-set-groupby' into cuco-set-…
PointKernel 6e54cd9
Merge remote-tracking branch 'upstream/branch-24.04' into cuco-set-gr…
PointKernel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@PointKernel You mentioned:
Do we need to adopt that here? Do we need an issue or TODO describing that optimization?
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.
Probably not. That requires nontrivial changes to the current code and the benefit is unclear, i.e., no users actually complained about the groupby performance with nested types. I'm inclined to look into it until we have the bandwidth or someone raises an issue about it. Does it sound good to you?
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’d like some kind of note in the code or an issue to make sure that we are aware of this optimization potential for the future. Otherwise, no action needed in terms of implementation.
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.
Make sense. Done in 56a2229