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.

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

Workaround for CUB segmented-sort bug with boolean keys #12217

Merged

Conversation

davidwendt
Copy link
Contributor

Description

Adds a workaround for cudf::segmented_sort when following the optimized path with a the key type of bool.
Here, the optimized path is avoided by checking the column-type.
A gtest is also included for this case.

The workaround can be removed once NVIDIA/cub#594 is fixed and available for libcudf.

Closes #12201

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@davidwendt davidwendt added bug Something isn't working 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Nov 21, 2022
@davidwendt davidwendt self-assigned this Nov 21, 2022
@davidwendt davidwendt requested a review from a team as a code owner November 21, 2022 23:17
@davidwendt davidwendt requested review from bdice and mythrocks and removed request for a team November 21, 2022 23:17
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.

Thanks for this workaround @davidwendt!

@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Base: 88.25% // Head: 88.25% // No change to project coverage 👍

Coverage data is based on head (cd6dff3) compared to base (21ba312).
Patch has no changes to coverable lines.

Additional details and impacted files
@@              Coverage Diff              @@
##           branch-22.12   #12217   +/-   ##
=============================================
  Coverage         88.25%   88.25%           
=============================================
  Files               137      137           
  Lines             22571    22571           
=============================================
  Hits              19921    19921           
  Misses             2650     2650           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@davidwendt
Copy link
Contributor Author

Waiting on Spark to verify this change passes their tests.

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.

Thanks, @davidwendt! Spark tests that were failing before this change are now passing.

@jolorunyomi jolorunyomi merged commit 49f983d into rapidsai:branch-22.12 Nov 22, 2022
@davidwendt davidwendt deleted the bug-cub-segmented-sort branch November 22, 2022 20:54
rapids-bot bot pushed a commit that referenced this pull request Nov 29, 2022
Fix in CUB DeviceSegmentedSort allows for workaround to removed. The CUB fix is applied as a patch in the libcudf build process.
Reference NVIDIA/cub#594 and #12217

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Elias Stehle (https://github.com/elstehle)
  - Robert Maynard (https://github.com/robertmaynard)

URL: #12234
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants