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

Fix unsanitized nulls produced by libcudf dictionary decode #14578

Merged
merged 4 commits into from
Dec 8, 2023

Conversation

davidwendt
Copy link
Contributor

Description

Fixes cudf::dictionary::decode logic to produced sanitized null entries for compound column types.

Reference: #14559 -- fixes many of the errors found here concerning dictionary column gtests.

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 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Dec 5, 2023
@davidwendt davidwendt self-assigned this Dec 5, 2023
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Dec 6, 2023
@davidwendt davidwendt marked this pull request as ready for review December 6, 2023 15:58
@davidwendt davidwendt requested a review from a team as a code owner December 6, 2023 15:58
@@ -30,6 +29,18 @@
namespace cudf {
namespace dictionary {
namespace detail {
namespace {
struct indices_handler_fn {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. Would there be value in naming this functor a "nullifying_handler" or some such?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actually nullifying happens far away in the gather function. This function simply returns indices.

Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@davidwendt
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit a8f071d into rapidsai:branch-24.02 Dec 8, 2023
67 checks passed
@davidwendt davidwendt deleted the dict-unsanitize-decode branch December 8, 2023 14:32
karthikeyann pushed a commit to karthikeyann/cudf that referenced this pull request Dec 12, 2023
…#14578)

Fixes `cudf::dictionary::decode` logic to produced sanitized null entries for compound column types.

Reference: rapidsai#14559 -- fixes many of the errors found here concerning dictionary column gtests.

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

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - MithunR (https://github.com/mythrocks)

URL: rapidsai#14578
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
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants