Skip to content

Use the aligned_resource_adaptor to allocate bloom filter device buffers#17758

Merged
rapids-bot[bot] merged 16 commits into
rapidsai:branch-25.02from
mhaseeb123:fix/align-bloom-filter-buffers
Jan 22, 2025
Merged

Use the aligned_resource_adaptor to allocate bloom filter device buffers#17758
rapids-bot[bot] merged 16 commits into
rapidsai:branch-25.02from
mhaseeb123:fix/align-bloom-filter-buffers

Conversation

@mhaseeb123

@mhaseeb123 mhaseeb123 commented Jan 17, 2025

Copy link
Copy Markdown
Member

Description

Related to #17164
Related to NVIDIA/cuCollections#660

This PR creates and uses a rmm::mr::aligned_resource_adapter to allocate device buffers for bloom filter data in accordance with bloom filter alignment requirements. This PR also updates the query_bloom_filter function to use the new bloom_filter_ref constructors introduced in NVIDIA/cuCollections#660.

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 Bot commented Jan 17, 2025

Copy link
Copy Markdown

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 17, 2025
@mhaseeb123 mhaseeb123 marked this pull request as ready for review January 17, 2025 01:53
@mhaseeb123 mhaseeb123 requested a review from a team as a code owner January 17, 2025 01:53
@mhaseeb123 mhaseeb123 added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change cuco cuCollections related issue labels Jan 17, 2025
@mhaseeb123 mhaseeb123 changed the title Use an aligned mr adapter to allocate bloom filter device buffers Use an aligned_resource_adaptor to allocate bloom filter device buffers Jan 17, 2025
Comment thread cpp/src/io/parquet/bloom_filter_reader.cu
@mhaseeb123 mhaseeb123 added DO NOT MERGE Hold off on merging; see PR for details and removed 3 - Ready for Review Ready for review by team labels Jan 17, 2025
@mhaseeb123 mhaseeb123 marked this pull request as draft January 17, 2025 02:22
@mhaseeb123 mhaseeb123 added 2 - In Progress Currently a work in progress and removed DO NOT MERGE Hold off on merging; see PR for details labels Jan 17, 2025
@mhaseeb123 mhaseeb123 changed the title Use an aligned_resource_adaptor to allocate bloom filter device buffers 🚧 Use an aligned_resource_adaptor to allocate bloom filter device buffers Jan 17, 2025
@copy-pr-bot

copy-pr-bot Bot commented Jan 17, 2025

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@bdice bdice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. It surprises me that this segfaulted the compiler in certain cases. We will need to file an internal bug report for this.

@mhaseeb123

mhaseeb123 commented Jan 17, 2025

Copy link
Copy Markdown
Member Author

Looks good to me. It surprises me that this segfaulted the compiler in certain cases. We will need to file an internal bug report for this.

This PR doesn't unfortunately fix the compiler segfault unfortunately, (NVIDIA/cuCollections#660) should do that instead. I will update this PR to use the new bloom_filter_ref constructors from NVIDIA/cuCollections#660 once the cuco is bumped rapidsai/rapids-cmake#744

@mhaseeb123 mhaseeb123 marked this pull request as ready for review January 17, 2025 23:44
@mhaseeb123 mhaseeb123 added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jan 17, 2025
@mhaseeb123 mhaseeb123 changed the title 🚧 Use an aligned_resource_adaptor to allocate bloom filter device buffers Use the aligned_resource_adaptor to allocate bloom filter device buffers Jan 17, 2025
@mhaseeb123

Copy link
Copy Markdown
Member Author

Locally verified with test data that the bloom filter is working as expected

@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 18, 2025

@PointKernel PointKernel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tested locally with the 12.0 pip unified container and the compilation finished without issues.

@mhaseeb123 great work! And thank you, @sleeepyjack for the prompt fix on the cuco end.

@mhaseeb123 mhaseeb123 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Needs Review Waiting for reviewer to review or respond labels Jan 22, 2025
@bdice

bdice commented Jan 22, 2025

Copy link
Copy Markdown
Contributor

/merge

@rapids-bot rapids-bot Bot merged commit 2a21f6c into rapidsai:branch-25.02 Jan 22, 2025
@mhaseeb123 mhaseeb123 deleted the fix/align-bloom-filter-buffers branch January 22, 2025 18:06
@GregoryKimball GregoryKimball removed this from libcudf Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working cuco cuCollections related issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants