Adopt cuco's parametric bloom filter policy in Parquet and streaming - #23049
Conversation
|
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. |
|
/ok to test 0c16a97 |
|
/ok to test 81828e6 |
|
/ok to test ba07b5c |
|
/ok to test 189ac84 |
|
/ok to test 763fb86 |
|
This is ready for review and the CMake changes will be reverted once the upstream #1044 gets in. |
mhaseeb123
left a comment
There was a problem hiding this comment.
Awesome work. LGTM (with test/debug artifacts removed of course)
wence-
left a comment
There was a problem hiding this comment.
With the caveat around the streaming detail usage.
Per [NVIDIA/cudf#23049 (comment)](NVIDIA/cudf#23049 (comment)), `parametric_filter_policy` no longer reads `typename Hash::argument_type`: `split_hash` is templated on the key type and deduces everything from the actual call, so hashers without that alias (e.g. cudf's `XXHash_64`) work directly. `hash_result_type` is fixed to `uint64_t` with the 64-bit assert moved into `split_hash`.
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces cudf's custom ChangesBloom filter policy migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmake/rapids_config.cmake`:
- Around line 37-40: Remove the temporary rapids-cmake test pin so main does not
always fetch from the personal fork/branch. The unconditional assignments to
rapids-cmake-repo and rapids-cmake-branch currently bypass the existing NOT
rapids-cmake-branch guard and override caller settings; delete or revert this
test block before merge. Use the rapids-cmake configuration logic in
rapids_config.cmake as the place to restore the normal override behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6ff23f16-b06c-4df9-9ea0-01cde3c29991
📒 Files selected for processing (6)
cmake/rapids_config.cmakecpp/libcudf_streaming/src/detail/device_bloom_filter.cucpp/src/io/parquet/arrow_filter_policy.cuhcpp/src/io/parquet/bloom_filter_reader.cucpp/src/join/mark_join.cuhcpp/tests/io/parquet_bloom_filter_test.cu
💤 Files with no reviewable changes (1)
- cpp/src/io/parquet/arrow_filter_policy.cuh
|
/merge |
Description
This PR migrates the Parquet reader and the streaming bloom filter onto cuco's new
parametric_filter_policy(bumped via rapidsai/rapids-cmake#1044). The Parquet reader defines a localarrow_filter_policyalias hashing keys withXXHash_64, and streaming uses the policy directly withidentity_hash; no shared cudf policy header is added. Depends on NVIDIA/cuCollections#825, which drops the hasherargument_typerequirement soXXHash_64can be used without an adapter.Checklist