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

Avoid instantiating bloom filter query function for nested and bool types #17753

Merged
merged 6 commits into from
Jan 17, 2025
Merged

Avoid instantiating bloom filter query function for nested and bool types #17753

merged 6 commits into from
Jan 17, 2025

Conversation

mhaseeb123
Copy link
Member

@mhaseeb123 mhaseeb123 commented Jan 15, 2025 •
edited by ttnghia
Loading

Description

This PR avoids instantiating the bloom filter query function for nested and boolean types avoiding a compiler bug and may also slightly improve compile time.

Checklist

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

@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jan 15, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jan 15, 2025
@mhaseeb123 mhaseeb123 changed the title Avoid instantiating bloom filter query for nested and bool types Avoid instantiating bloom filter query function for nested and bool types Jan 15, 2025
@mhaseeb123 mhaseeb123 added bug Something isn't working 2 - In Progress Currently a work in progress non-breaking Non-breaking change labels Jan 15, 2025
@mhaseeb123 mhaseeb123 marked this pull request as ready for review January 15, 2025 23:22
@mhaseeb123 mhaseeb123 requested a review from a team as a code owner January 15, 2025 23:22
@mhaseeb123 mhaseeb123 added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jan 15, 2025
@mhaseeb123 mhaseeb123 added 4 - Needs Review Waiting for reviewer to review or respond and removed 3 - Ready for Review Ready for review by team labels Jan 16, 2025
PointKernel
PointKernel requested changes Jan 16, 2025
View reviewed changes
Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

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

Putting this on hold as the issue hasn't been fixed yet

@mhaseeb123
Copy link
Member Author

mhaseeb123 commented Jan 16, 2025 •
edited
Loading

Putting this on hold as the issue hasn't been fixed yet

Yep, same. Added a DO NOT MERGE

@mhaseeb123 mhaseeb123 added DO NOT MERGE Hold off on merging; see PR for details and removed 4 - Needs Review Waiting for reviewer to review or respond labels Jan 16, 2025
@bdice bdice mentioned this pull request Jan 16, 2025
Merged
ttnghia
ttnghia approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@ttnghia ttnghia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Although this does not fix the reported compile issue, such issue can be fixed only by either removing the usage of cuco::bloom_filter_ref in libcudf or fixing cuco::bloom_filter_ref in cuco.

This change is good for its purpose.

@mhaseeb123 mhaseeb123 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed DO NOT MERGE Hold off on merging; see PR for details labels Jan 16, 2025
@mhaseeb123
Copy link
Member Author

Agreed!

@mhaseeb123
Copy link
Member Author

/merge

@mhaseeb123 mhaseeb123 mentioned this pull request Jan 17, 2025
Closed
@rapids-bot rapids-bot bot merged commit 67b9913 into rapidsai:branch-25.02 Jan 17, 2025
106 checks passed
@mhaseeb123 mhaseeb123 deleted the fix/bf-query-instantiation branch January 17, 2025 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ttnghia ttnghia ttnghia approved these changes

@PointKernel PointKernel PointKernel approved these changes

Assignees

@mhaseeb123 mhaseeb123

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
libcudf
Status: Landed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mhaseeb123 @ttnghia @PointKernel