Rework approach to cudf-streaming bloom filter sizing - #23067
Conversation
github-actions
Bot
added
libcudf
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates bloom filter sizing from block counts to aligned byte sizes across C++ and Python APIs. Adds a shared Arrow-compatible policy alias, updates device allocation and merge paths, adjusts benchmarks and integrations, and adds C++ and Python validation tests. ChangesBloom filter byte-size API migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/libcudf_streaming/tests/streaming/test_bloom_filter.cu (1)
38-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing zero-size edge case for
device_bloom_filter::storage.The doc contract requires
filter_sizeto be a "positive multiple of the filter block size," but this test only exercises the unaligned (65) case, not zero. The siblingbloom_filtertest (test_bloom_filter_config.cpp) explicitly coversfilter_size == 0; add the same coverage here fordevice_bloom_filter::storage.As per path instructions,
cpp/**/*test*.{cu,cpp}guideline: "Test functions must cover edge cases: empty input, null values, sliced columns, boundary sizes, multi-block sizes."✅ Suggested addition
EXPECT_THROW(cudf_streaming::detail::device_bloom_filter::storage( unaligned_size, stream, cudf::get_current_device_resource_ref()), std::logic_error); + + EXPECT_THROW(cudf_streaming::detail::device_bloom_filter::storage( + std::size_t{0}, stream, cudf::get_current_device_resource_ref()), + std::logic_error);🤖 Prompt for 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. In `@cpp/libcudf_streaming/tests/streaming/test_bloom_filter.cu` around lines 38 - 56, The DeviceBloomFilterTest::RequiresAlignedStorageSize coverage is missing the zero-size edge case for cudf_streaming::detail::device_bloom_filter::storage. Add a check that passing filter_size == 0 to device_bloom_filter::storage with the current stream and resource reference throws std::logic_error, matching the contract and the sibling bloom_filter tests. Keep the existing unaligned and aligned assertions, and locate the change in DeviceBloomFilterTest and device_bloom_filter::storage usage.Source: Path instructions
🤖 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.
Nitpick comments:
In `@cpp/libcudf_streaming/tests/streaming/test_bloom_filter.cu`:
- Around line 38-56: The DeviceBloomFilterTest::RequiresAlignedStorageSize
coverage is missing the zero-size edge case for
cudf_streaming::detail::device_bloom_filter::storage. Add a check that passing
filter_size == 0 to device_bloom_filter::storage with the current stream and
resource reference throws std::logic_error, matching the contract and the
sibling bloom_filter tests. Keep the existing unaligned and aligned assertions,
and locate the change in DeviceBloomFilterTest and device_bloom_filter::storage
usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4a5be620-43d2-42b8-8af4-8b37024da25d
📒 Files selected for processing (16)
cpp/libcudf_streaming/benchmarks/streaming/ndsh/q03.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q04.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q21.cppcpp/libcudf_streaming/include/cudf_streaming/bloom_filter.hppcpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hppcpp/libcudf_streaming/include/cudf_streaming/detail/large_arrow_filter_policy.cuhcpp/libcudf_streaming/src/bloom_filter.cppcpp/libcudf_streaming/src/detail/device_bloom_filter.cucpp/libcudf_streaming/tests/CMakeLists.txtcpp/libcudf_streaming/tests/streaming/test_bloom_filter.cucpp/libcudf_streaming/tests/streaming/test_bloom_filter_config.cpppython/cudf_polars/cudf_polars/streaming/actor_graph/join.pypython/cudf_streaming/cudf_streaming/bloom_filter.pxdpython/cudf_streaming/cudf_streaming/bloom_filter.pyipython/cudf_streaming/cudf_streaming/bloom_filter.pyxpython/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py
d23c38a to
ab98fee
Compare
|
Should wait for #23049 and use the new parametric policy that PR introduces |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/libcudf_streaming/tests/streaming/test_bloom_filter_config.cpp (1)
14-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnonymous namespace wraps TEST macros.
As per coding guidelines,
"Test code must be in the global namespace, not in custom namespaces". Thenamespace { ... }block here wraps bothTESTcases; consider moving them to the global namespace for consistency with this rule.🤖 Prompt for 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. In `@cpp/libcudf_streaming/tests/streaming/test_bloom_filter_config.cpp` around lines 14 - 37, The BloomFilterTest cases are currently defined inside an anonymous namespace, but TEST macros must live in the global namespace. Remove the surrounding anonymous namespace in test_bloom_filter_config.cpp so AlignsStorageSize and RequiresAlignedStorageSize are declared at global scope, keeping the existing cudf_streaming::bloom_filter references unchanged.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@cpp/libcudf_streaming/tests/streaming/test_bloom_filter_config.cpp`:
- Around line 14-37: The BloomFilterTest cases are currently defined inside an
anonymous namespace, but TEST macros must live in the global namespace. Remove
the surrounding anonymous namespace in test_bloom_filter_config.cpp so
AlignsStorageSize and RequiresAlignedStorageSize are declared at global scope,
keeping the existing cudf_streaming::bloom_filter references unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df97bac8-1d0a-4448-bbf9-ad0d2f12a4aa
📒 Files selected for processing (12)
cpp/libcudf_streaming/benchmarks/streaming/ndsh/q03.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q04.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q21.cppcpp/libcudf_streaming/include/cudf_streaming/bloom_filter.hppcpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hppcpp/libcudf_streaming/include/cudf_streaming/detail/large_arrow_filter_policy.cuhcpp/libcudf_streaming/src/bloom_filter.cppcpp/libcudf_streaming/src/detail/device_bloom_filter.cucpp/libcudf_streaming/tests/CMakeLists.txtcpp/libcudf_streaming/tests/streaming/test_bloom_filter.cucpp/libcudf_streaming/tests/streaming/test_bloom_filter_config.cpppython/cudf_polars/cudf_polars/streaming/actor_graph/join.py
🚧 Files skipped from review as they are similar to previous changes (11)
- cpp/libcudf_streaming/include/cudf_streaming/detail/large_arrow_filter_policy.cuh
- python/cudf_polars/cudf_polars/streaming/actor_graph/join.py
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q03.cpp
- cpp/libcudf_streaming/tests/CMakeLists.txt
- cpp/libcudf_streaming/tests/streaming/test_bloom_filter.cu
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q21.cpp
- cpp/libcudf_streaming/src/bloom_filter.cpp
- cpp/libcudf_streaming/include/cudf_streaming/bloom_filter.hpp
- cpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hpp
- cpp/libcudf_streaming/src/detail/device_bloom_filter.cu
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q04.cpp
ab98fee to
51d84d7
Compare
TomAugspurger
left a comment
TomAugspurger
left a comment
There was a problem hiding this comment.
Python changes look good, but one question about a rapidsmpf import.
| from libc.stdint cimport uint64_t | ||
| from libcpp.memory cimport shared_ptr, unique_ptr | ||
|
|
||
| from rapidsmpf._detail.exception_handling cimport ex_handler |
There was a problem hiding this comment.
Can we avoid the ._detail API import from rapidsmpf here, perhaps by making it public in rapidsmpf (and updating the usage here in another PR)?
There was a problem hiding this comment.
I will do this separately.
PointKernel
left a comment
PointKernel
left a comment
There was a problem hiding this comment.
LGTM. Some of this logic could eventually be moved into cuco, but I don't think it's worth blocking this PR.
| * | ||
| * Maximum valid filter size in bytes. | ||
| */ | ||
| [[nodiscard]] static std::size_t max_size() noexcept; |
There was a problem hiding this comment.
@sleeepyjack I think this is the kind of utility, along with the corresponding validation checks, that we should expose in cuco rather than expecting users to implement themselves. Similar to how we provide utilities/constructors for configuring HLL from different perspectives, such as target standard deviation or sketch size in bytes, we could offer the same level of convenience here.
51d84d7 to
d115c3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cpp/libcudf_streaming/include/cudf_streaming/bloom_filter.hpp`:
- Around line 58-64: Update the documentation for aligned_size in the bloom
filter interface to describe only storage alignment: state that it returns the
largest aligned size no greater than the input, including that small inputs may
produce zero, and clarify that callers must separately validate positivity and
filter policy limits.
In `@cpp/libcudf_streaming/tests/streaming/test_bloom_filter_config.cpp`:
- Around line 17-22: Update the AlignsStorageSize test expectations for
cudf_streaming::bloom_filter::aligned_size to verify ceiling alignment: assert
31 maps to 32, 32 remains 32, and 65 maps to 96.
🪄 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: 6ca63bf8-4fea-49c1-8f17-54e2cf0c4e05
📒 Files selected for processing (12)
cpp/include/cudf/reduction/bloom_filter.cuhcpp/libcudf_streaming/benchmarks/streaming/ndsh/q03.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q04.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q21.cppcpp/libcudf_streaming/include/cudf_streaming/bloom_filter.hppcpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hppcpp/libcudf_streaming/src/bloom_filter.cppcpp/libcudf_streaming/src/detail/device_bloom_filter.cucpp/libcudf_streaming/tests/CMakeLists.txtcpp/libcudf_streaming/tests/streaming/test_bloom_filter.cucpp/libcudf_streaming/tests/streaming/test_bloom_filter_config.cppcpp/src/io/parquet/bloom_filter_reader.cu
🚧 Files skipped from review as they are similar to previous changes (9)
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q04.cpp
- cpp/libcudf_streaming/tests/streaming/test_bloom_filter.cu
- cpp/libcudf_streaming/tests/CMakeLists.txt
- cpp/include/cudf/reduction/bloom_filter.cuh
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q03.cpp
- cpp/libcudf_streaming/src/bloom_filter.cpp
- cpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hpp
- cpp/src/io/parquet/bloom_filter_reader.cu
- cpp/libcudf_streaming/src/detail/device_bloom_filter.cu
wence-
added
improvement
Rather than asking for a fitting number of blocks for an size, just require the user to provide the size of the filter directly. Since it must be a multiple of the storage type, also offer an aligned_size utility that takes an input and makes that happen. Additionally, share the new cuco parametric filter policy across cudf and cudf-streaming.
d115c3f to
ff29c5a
Compare
wence-
changed the title
|
@PointKernel @mhaseeb123 are you happy with the file naming choice for the new shared filter policy |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py (1)
195-200: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMigrate this test from the removed block-count API.
Line 199 still passes
num_filter_blocks=BloomFilter.fitting_num_blocks(...); this migration replaces both withfilter_size, so the regression test will fail against the updated binding.Proposed fix
- num_filter_blocks=BloomFilter.fitting_num_blocks(1 << 20), + filter_size=1 << 20,🤖 Prompt for 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. In `@python/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py` around lines 195 - 200, Update the BloomFilter construction in test_bloom_filter.py to replace the removed num_filter_blocks argument and BloomFilter.fitting_num_blocks(...) call with the new filter_size argument, preserving the intended 1 << 20 filter size for the regression test.
🤖 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 `@python/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py`:
- Around line 45-48: Update the assertions in test_aligned_size to match
BloomFilter.aligned_size’s round-up contract: expect 31 to produce 32 and 65 to
produce 96, while retaining the existing 32-to-32 expectation.
---
Outside diff comments:
In `@python/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py`:
- Around line 195-200: Update the BloomFilter construction in
test_bloom_filter.py to replace the removed num_filter_blocks argument and
BloomFilter.fitting_num_blocks(...) call with the new filter_size argument,
preserving the intended 1 << 20 filter size for the regression test.
🪄 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: c42824ae-a351-4dee-8c4a-d475aae417c5
📒 Files selected for processing (18)
cpp/include/cudf/reduction/bloom_filter.cuhcpp/libcudf_streaming/benchmarks/streaming/ndsh/q03.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q04.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q21.cppcpp/libcudf_streaming/include/cudf_streaming/bloom_filter.hppcpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hppcpp/libcudf_streaming/src/bloom_filter.cppcpp/libcudf_streaming/src/detail/device_bloom_filter.cucpp/libcudf_streaming/tests/CMakeLists.txtcpp/libcudf_streaming/tests/streaming/test_bloom_filter.cucpp/libcudf_streaming/tests/streaming/test_bloom_filter_config.cppcpp/src/io/parquet/bloom_filter_reader.cucpp/tests/io/parquet_bloom_filter_test.cupython/cudf_polars/cudf_polars/streaming/actor_graph/join.pypython/cudf_streaming/cudf_streaming/bloom_filter.pxdpython/cudf_streaming/cudf_streaming/bloom_filter.pyipython/cudf_streaming/cudf_streaming/bloom_filter.pyxpython/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py
🚧 Files skipped from review as they are similar to previous changes (15)
- cpp/include/cudf/reduction/bloom_filter.cuh
- cpp/libcudf_streaming/tests/CMakeLists.txt
- cpp/tests/io/parquet_bloom_filter_test.cu
- cpp/libcudf_streaming/tests/streaming/test_bloom_filter.cu
- python/cudf_polars/cudf_polars/streaming/actor_graph/join.py
- cpp/libcudf_streaming/src/bloom_filter.cpp
- python/cudf_streaming/cudf_streaming/bloom_filter.pyi
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q21.cpp
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q03.cpp
- python/cudf_streaming/cudf_streaming/bloom_filter.pyx
- cpp/src/io/parquet/bloom_filter_reader.cu
- cpp/libcudf_streaming/src/detail/device_bloom_filter.cu
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q04.cpp
- cpp/libcudf_streaming/include/cudf_streaming/bloom_filter.hpp
- cpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hpp
| def test_aligned_size() -> None: | ||
| assert BloomFilter.aligned_size(31) == 0 | ||
| assert BloomFilter.aligned_size(32) == 32 | ||
| assert BloomFilter.aligned_size(65) == 64 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the aligned_size expectations to round up.
Lines 46-48 assert round-down results (31 → 0, 65 → 64), but this API’s new contract rounds up to the storage-block multiple. Expect 31 → 32 and 65 → 96.
Proposed fix
- assert BloomFilter.aligned_size(31) == 0
+ assert BloomFilter.aligned_size(31) == 32
assert BloomFilter.aligned_size(32) == 32
- assert BloomFilter.aligned_size(65) == 64
+ assert BloomFilter.aligned_size(65) == 96📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_aligned_size() -> None: | |
| assert BloomFilter.aligned_size(31) == 0 | |
| assert BloomFilter.aligned_size(32) == 32 | |
| assert BloomFilter.aligned_size(65) == 64 | |
| def test_aligned_size() -> None: | |
| assert BloomFilter.aligned_size(31) == 32 | |
| assert BloomFilter.aligned_size(32) == 32 | |
| assert BloomFilter.aligned_size(65) == 96 |
🤖 Prompt for 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.
In `@python/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py` around lines
45 - 48, Update the assertions in test_aligned_size to match
BloomFilter.aligned_size’s round-up contract: expect 31 to produce 32 and 65 to
produce 96, while retaining the existing 32-to-32 expectation.
Looks good. As for the location, the public |
LGTM! |
|
/merge |
f0a24f4
into
NVIDIA:release/26.08
8 participants
Description
Rather than asking for a fitting number of blocks for an size, just require
the user to provide the size of the filter directly. Since it must be a
multiple of the storage type, also offer an aligned_size utility that takes
an input and makes that happen.
Additionally, share the new cuco parametric filter policy across cudf and
cudf-streaming.
Checklist