Add approx_distinct_count API to cudf_streaming - #23522
Conversation
github-actions
Bot
added
libcudf
wence-
added
improvement
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds distributed approximate distinct-count estimation in libcudf streaming, exposes it through Cython, and integrates it with Polars chunk sampling. The implementation supports selected columns, sampled-chunk forwarding, distributed reduction, precision validation, and completeness tracking. ChangesCardinality estimation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 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.
Actionable comments posted: 3
🧹 Nitpick comments (7)
cpp/libcudf_streaming/src/detail/approx_distinct_count.cu (1)
15-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
CUDF_KERNELinstead of raw__global__.Both kernels declare
__global__directly. The guidelines require theCUDF_KERNELmacro, preferably with__launch_bounds__.♻️ Proposed change
+#include <cudf/detail/utilities/integer_utils.hpp> +#include <cudf/utilities/export.hpp> + -__global__ void set_value_kernel(std::byte* data, std::size_t offset, std::uint64_t value) +CUDF_KERNEL void __launch_bounds__(1) + set_value_kernel(std::byte* data, std::size_t offset, std::uint64_t value) { if (threadIdx.x == 0) { *reinterpret_cast<std::uint64_t*>(data + offset) = value; } } -__global__ void add_values_kernel(std::byte const* left, std::byte* right, std::size_t offset) +CUDF_KERNEL void __launch_bounds__(1) + add_values_kernel(std::byte const* left, std::byte* right, std::size_t offset) {Use the header that actually defines
CUDF_KERNELin this repository instead of the placeholder includes above.As per coding guidelines: "Use
CUDF_KERNELrather than raw__global__, preferably with__launch_bounds__."🤖 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/src/detail/approx_distinct_count.cu` around lines 15 - 26, Replace the raw __global__ qualifiers on set_value_kernel and add_values_kernel with the repository’s CUDF_KERNEL macro, adding __launch_bounds__ if consistent with nearby kernel conventions. Include the header that defines CUDF_KERNEL and preserve both kernels’ existing behavior.Source: Coding guidelines
cpp/libcudf_streaming/include/cudf_streaming/approx_distinct_count.hpp (1)
104-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the estimator lifetime requirement for the returned actor.
estimateis a coroutine member function. The coroutine frame copies the arguments, but it stores onlythisfor the member state. The estimator object must stay alive until the actor completes, otherwise the coroutine dereferences a destroyed object after a suspension point. Add this requirement to the doc block so callers of the Python binding and the Polars sampler keep the estimator alive.📝 Proposed documentation addition
* `@param` column_indices Columns whose row tuples are counted. An empty vector selects all * columns. * `@return` Coroutine representing the estimation. + * + * `@note` The estimator must outlive the returned actor. The coroutine accesses estimator state + * after suspension points. */🤖 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/include/cudf_streaming/approx_distinct_count.hpp` around lines 104 - 108, Update the documentation for the coroutine member function estimate to state that the estimator instance must remain alive until the returned actor completes, because the coroutine retains this across suspension points. Ensure the requirement is visible to callers, including Python bindings and the Polars sampler, without changing the function signature or behavior.cpp/libcudf_streaming/include/cudf_streaming/detail/approx_distinct_count.hpp (1)
15-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd doxygen blocks and state the alignment precondition.
Both declarations lack doxygen comments. The corresponding kernels in
src/detail/approx_distinct_count.cureinterpretdata + offsetasstd::uint64_t*, sooffsetmust produce an 8-byte aligned address. Document that precondition and the device-memory requirement for the pointers.📝 Proposed documentation
+/** + * `@brief` Write a 64-bit value into device memory at a byte offset. + * + * `@param` data Device pointer to the target storage. + * `@param` offset Byte offset into `@p` data. Must yield an 8-byte aligned address. + * `@param` value Value to write. + * `@param` stream CUDA stream used for the write. + */ void set_value(std::byte* data, std::size_t offset, std::uint64_t value, rmm::cuda_stream_view stream); +/** + * `@brief` Add the 64-bit value at `@p` offset in `@p` left into `@p` right. + * + * `@param` left Device pointer to the source storage. + * `@param` right Device pointer to the destination storage. + * `@param` offset Byte offset into both buffers. Must yield an 8-byte aligned address. + * `@param` stream CUDA stream used for the update. + */ void add_values(std::byte const* left, std::byte* right, std::size_t offset, rmm::cuda_stream_view stream);As per coding guidelines: "Use doxygen as a documentation generator and linter for C++ and CUDA code."
🤖 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/include/cudf_streaming/detail/approx_distinct_count.hpp` around lines 15 - 23, Add Doxygen comments for set_value and add_values describing their parameters, including that the data pointers reference device memory and offset must produce an 8-byte-aligned address for the uint64_t access; document the stream parameter and each function’s operation without changing their declarations or behavior.Source: Coding guidelines
cpp/libcudf_streaming/src/approx_distinct_count.cpp (1)
6-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude a header for every used symbol in the new sources. Both new translation units rely on transitive includes for symbols they name directly. The shared root cause is incomplete include-what-you-use hygiene in this PR.
cpp/libcudf_streaming/src/approx_distinct_count.cpp#L6-L29: add<stdexcept>forstd::invalid_argument(line 55) and<cuda/std/cstddef>forcuda::std::byte(lines 110, 142, 146, 158).cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp#L6-L23: add<cstddef>forstd::size_t,cudf/column/column.hppforcudf::column, the rapidsmpf header that declaressafe_cast, andrapidsmpf/streaming/core/channel.hppfor channel creation.As per coding guidelines: "include headers directly for every used symbol without unused or incorrectly styled includes."
🤖 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/src/approx_distinct_count.cpp` around lines 6 - 29, Update both affected translation units to include headers directly for every symbol they use: in cpp/libcudf_streaming/src/approx_distinct_count.cpp#L6-L29 add <stdexcept> for std::invalid_argument and <cuda/std/cstddef> for cuda::std::byte; in cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp#L6-L23 add <cstddef> for std::size_t, cudf/column/column.hpp for cudf::column, the rapidsmpf header declaring safe_cast, and rapidsmpf/streaming/core/channel.hpp for channel creation. Remove or avoid any unused or incorrectly styled includes.Source: Coding guidelines
cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp (1)
126-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the valid precision boundaries and for null inputs.
RejectsInvalidPrecisionchecks 3 and 19. Add cases for 4 and 18 to pin the accepted range. The public header states that nulls and NaNs enter the sketch. Add a test with a null-containing column to lock that behavior.🤖 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_approx_distinct_count.cpp` around lines 126 - 134, The RejectsInvalidPrecision test currently only verifies that precision values 3 and 19 are rejected, leaving the valid boundary conditions untested. Add test cases that verify precision values 4 and 18 are accepted without throwing exceptions to establish the correct precision range. Additionally, create a separate test case that constructs a cardinality_estimator with a column containing null values to verify the documented behavior that nulls and NaNs are properly accepted and processed by the sketch.python/cudf_streaming/cudf_streaming/tests/test_approx_distinct_count.py (1)
120-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
column_indicesand for null values.The
sample()helper acceptscolumn_indices, but no test passes a non-empty value. The selected-columns path of the new API is therefore untested. Add a test with a multi-column table that estimates on a subset of columns, and assert that the distinct count reflects only those columns. Add a case with an all-null column as well, because the native actor fixesnull_handlingandnan_handlingand that behavior is not asserted anywhere.As per coding guidelines: "Ensure test files provide comprehensive edge case coverage (empty, all-null, single-element, mixed types)".
🤖 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_approx_distinct_count.py` around lines 120 - 131, Extend coverage around sample() by adding tests for non-empty column_indices using a multi-column table, asserting distinct_count reflects only the selected columns, and for an all-null column to verify null handling. Anchor the additions near test_estimate_forwards_input and follow existing Context, Communicator, make_table, and assertion patterns; include relevant empty, single-element, or mixed-type cases only where needed to exercise these paths.Source: Coding guidelines
python/cudf_streaming/cudf_streaming/approx_distinct_count.pyi (1)
13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a concrete static signature for
CardinalityEstimate.from_message. The implementation is a@staticmethodand always constructsCardinalityEstimate; usemessage: Message[CardinalityEstimate]and returnCardinalityEstimate.🤖 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/approx_distinct_count.pyi` around lines 13 - 16, Update the from_message declaration in the CardinalityEstimate stub to use the concrete static signature: remove the classmethod receiver, accept Message[CardinalityEstimate], and retain CardinalityEstimate as the return type, matching the implementation’s `@staticmethod` behavior.
🤖 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/src/approx_distinct_count.cpp`:
- Around line 162-167: Update the row-count transfer in the distinct-count
estimation path to use a pinned host vector and the span-based cuda_memcpy API
instead of copying into stack variable row_count via memcpy_async. Preserve the
existing device offset, byte size, stream synchronization, and returned pair by
extracting the copied value from the pinned vector.
In `@python/cudf_polars/cudf_polars/streaming/actor_graph/utils.py`:
- Around line 1061-1068: Update the Returns section of the affected docstring so
the third return type is int instead of rows, and correct its description to say
“Number of rows in the sampled chunks.”
- Around line 1134-1144: Guard the cardinality-estimator result in the
`cardinality_estimator` branch before calling
`CardinalityEstimate.from_message(msg)`, raising a clear `RuntimeError` when
`msg is None`; also verify or adjust the `ch_cardinality` consumption so the
rapidsmpf channel cannot leave the estimator’s output drain blocked, following
the established receive-loop and `drain()` pattern where required.
---
Nitpick comments:
In `@cpp/libcudf_streaming/include/cudf_streaming/approx_distinct_count.hpp`:
- Around line 104-108: Update the documentation for the coroutine member
function estimate to state that the estimator instance must remain alive until
the returned actor completes, because the coroutine retains this across
suspension points. Ensure the requirement is visible to callers, including
Python bindings and the Polars sampler, without changing the function signature
or behavior.
In
`@cpp/libcudf_streaming/include/cudf_streaming/detail/approx_distinct_count.hpp`:
- Around line 15-23: Add Doxygen comments for set_value and add_values
describing their parameters, including that the data pointers reference device
memory and offset must produce an 8-byte-aligned address for the uint64_t
access; document the stream parameter and each function’s operation without
changing their declarations or behavior.
In `@cpp/libcudf_streaming/src/approx_distinct_count.cpp`:
- Around line 6-29: Update both affected translation units to include headers
directly for every symbol they use: in
cpp/libcudf_streaming/src/approx_distinct_count.cpp#L6-L29 add <stdexcept> for
std::invalid_argument and <cuda/std/cstddef> for cuda::std::byte; in
cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp#L6-L23 add
<cstddef> for std::size_t, cudf/column/column.hpp for cudf::column, the
rapidsmpf header declaring safe_cast, and rapidsmpf/streaming/core/channel.hpp
for channel creation. Remove or avoid any unused or incorrectly styled includes.
In `@cpp/libcudf_streaming/src/detail/approx_distinct_count.cu`:
- Around line 15-26: Replace the raw __global__ qualifiers on set_value_kernel
and add_values_kernel with the repository’s CUDF_KERNEL macro, adding
__launch_bounds__ if consistent with nearby kernel conventions. Include the
header that defines CUDF_KERNEL and preserve both kernels’ existing behavior.
In `@cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp`:
- Around line 126-134: The RejectsInvalidPrecision test currently only verifies
that precision values 3 and 19 are rejected, leaving the valid boundary
conditions untested. Add test cases that verify precision values 4 and 18 are
accepted without throwing exceptions to establish the correct precision range.
Additionally, create a separate test case that constructs a
cardinality_estimator with a column containing null values to verify the
documented behavior that nulls and NaNs are properly accepted and processed by
the sketch.
In `@python/cudf_streaming/cudf_streaming/approx_distinct_count.pyi`:
- Around line 13-16: Update the from_message declaration in the
CardinalityEstimate stub to use the concrete static signature: remove the
classmethod receiver, accept Message[CardinalityEstimate], and retain
CardinalityEstimate as the return type, matching the implementation’s
`@staticmethod` behavior.
In `@python/cudf_streaming/cudf_streaming/tests/test_approx_distinct_count.py`:
- Around line 120-131: Extend coverage around sample() by adding tests for
non-empty column_indices using a multi-column table, asserting distinct_count
reflects only the selected columns, and for an all-null column to verify null
handling. Anchor the additions near test_estimate_forwards_input and follow
existing Context, Communicator, make_table, and assertion patterns; include
relevant empty, single-element, or mixed-type cases only where needed to
exercise these paths.
🪄 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: b30b6143-e522-46e3-b763-8d93ef55e622
📒 Files selected for processing (16)
cpp/libcudf_streaming/CMakeLists.txtcpp/libcudf_streaming/include/cudf_streaming/approx_distinct_count.hppcpp/libcudf_streaming/include/cudf_streaming/detail/approx_distinct_count.hppcpp/libcudf_streaming/src/approx_distinct_count.cppcpp/libcudf_streaming/src/detail/approx_distinct_count.cucpp/libcudf_streaming/tests/CMakeLists.txtcpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpppython/cudf_polars/cudf_polars/streaming/actor_graph/join.pypython/cudf_polars/cudf_polars/streaming/actor_graph/utils.pypython/cudf_streaming/CMakeLists.txtpython/cudf_streaming/cudf_streaming/__init__.pxdpython/cudf_streaming/cudf_streaming/__init__.pypython/cudf_streaming/cudf_streaming/approx_distinct_count.pxdpython/cudf_streaming/cudf_streaming/approx_distinct_count.pyipython/cudf_streaming/cudf_streaming/approx_distinct_count.pyxpython/cudf_streaming/cudf_streaming/tests/test_approx_distinct_count.py
2d3a545 to
956135a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cpp/libcudf_streaming/src/approx_distinct_count.cpp (1)
6-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct headers for referenced types.
Both files rely on transitive headers. This makes builds depend on unrelated header internals.
cpp/libcudf_streaming/src/approx_distinct_count.cpp#L6-L31: Add<cuda/std/cstddef>forcuda::std::byte,<rmm/device_buffer.hpp>forrmm::device_buffer, and<stdexcept>forstd::invalid_argument.cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp#L16-L23: Add<cstddef>forstd::size_tand<stdexcept>forstd::invalid_argument.As per coding guidelines: "include headers directly for every used symbol without unused or incorrectly styled includes."
🤖 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/src/approx_distinct_count.cpp` around lines 6 - 31, Update the includes in cpp/libcudf_streaming/src/approx_distinct_count.cpp (lines 6-31) to directly include <cuda/std/cstddef> for cuda::std::byte, <rmm/device_buffer.hpp> for rmm::device_buffer, and <stdexcept> for std::invalid_argument. Also update cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp (lines 16-23) to directly include <cstddef> for std::size_t and <stdexcept> for std::invalid_argument, without adding unused headers.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.
Inline comments:
In `@cpp/libcudf_streaming/src/approx_distinct_count.cpp`:
- Around line 165-171: Update the row-count copy in the surrounding
distinct-count implementation to remove the unused row_count variable and pass
tmp.size() as the cudf::device_span element count instead of sizeof(row_count),
keeping the one-element destination and cuda_memcpy flow unchanged.
In `@cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp`:
- Around line 33-34: Update the test setup around values in
test_approx_distinct_count to copy the original range into a separate container
before appending it, avoiding self-range insertion while preserving the
duplicated values.
---
Nitpick comments:
In `@cpp/libcudf_streaming/src/approx_distinct_count.cpp`:
- Around line 6-31: Update the includes in
cpp/libcudf_streaming/src/approx_distinct_count.cpp (lines 6-31) to directly
include <cuda/std/cstddef> for cuda::std::byte, <rmm/device_buffer.hpp> for
rmm::device_buffer, and <stdexcept> for std::invalid_argument. Also update
cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp (lines
16-23) to directly include <cstddef> for std::size_t and <stdexcept> for
std::invalid_argument, without adding unused headers.
🪄 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: 5a4f935b-a9b9-4931-83ac-146b3fd69753
📒 Files selected for processing (16)
cpp/libcudf_streaming/CMakeLists.txtcpp/libcudf_streaming/include/cudf_streaming/approx_distinct_count.hppcpp/libcudf_streaming/include/cudf_streaming/detail/approx_distinct_count.hppcpp/libcudf_streaming/src/approx_distinct_count.cppcpp/libcudf_streaming/src/detail/approx_distinct_count.cucpp/libcudf_streaming/tests/CMakeLists.txtcpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpppython/cudf_polars/cudf_polars/streaming/actor_graph/join.pypython/cudf_polars/cudf_polars/streaming/actor_graph/utils.pypython/cudf_streaming/CMakeLists.txtpython/cudf_streaming/cudf_streaming/__init__.pxdpython/cudf_streaming/cudf_streaming/__init__.pypython/cudf_streaming/cudf_streaming/approx_distinct_count.pxdpython/cudf_streaming/cudf_streaming/approx_distinct_count.pyipython/cudf_streaming/cudf_streaming/approx_distinct_count.pyxpython/cudf_streaming/cudf_streaming/tests/test_approx_distinct_count.py
🚧 Files skipped from review as they are similar to previous changes (13)
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pyi
- python/cudf_streaming/cudf_streaming/init.py
- cpp/libcudf_streaming/src/detail/approx_distinct_count.cu
- cpp/libcudf_streaming/CMakeLists.txt
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pxd
- python/cudf_streaming/CMakeLists.txt
- python/cudf_polars/cudf_polars/streaming/actor_graph/join.py
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pyx
- python/cudf_polars/cudf_polars/streaming/actor_graph/utils.py
- python/cudf_streaming/cudf_streaming/init.pxd
- cpp/libcudf_streaming/tests/CMakeLists.txt
- python/cudf_streaming/cudf_streaming/tests/test_approx_distinct_count.py
- cpp/libcudf_streaming/include/cudf_streaming/approx_distinct_count.hpp
956135a to
d4b5b5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cpp/libcudf_streaming/src/detail/approx_distinct_count.cu (1)
15-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
CUDF_KERNELfor both kernel declarations.Line 15 and Line 20 use raw
__global__declarations. Replace both declarations withCUDF_KERNEL. Include the header that directly declaresCUDF_KERNEL.As per coding guidelines, “Use
CUDF_KERNELrather than raw__global__.”🤖 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/src/detail/approx_distinct_count.cu` around lines 15 - 22, Replace the raw __global__ declarations for set_value_kernel and add_values_kernel with CUDF_KERNEL, and include the header that directly declares CUDF_KERNEL.Source: Coding guidelines
cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp (2)
117-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the forwarded table payload.
The test checks only
sampled.front().sequence_number(). It does not prove that the forwarded message contains the original table. Release the sampledtable_chunkand verify its row count, column count, and values.🤖 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_approx_distinct_count.cpp` around lines 117 - 123, Extend the test around sampled.front() to release the forwarded table_chunk payload and assert the original table’s row count, column count, and values, while retaining the existing sequence_number check and estimate assertions.
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct standard-library headers.
Add
<cstddef>,<stdexcept>, and<utility>for the direct uses ofstd::size_t,std::invalid_argument, andstd::move.🤖 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_approx_distinct_count.cpp` around lines 21 - 23, Add the direct standard-library headers <cstddef>, <stdexcept>, and <utility> to support the file’s uses of std::size_t, std::invalid_argument, and std::move.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.
Inline comments:
In `@cpp/libcudf_streaming/src/detail/approx_distinct_count.cu`:
- Around line 27-36: Add RAPIDSMPF_EXPECTS validation for non-null device
pointers at the start of both set_value and add_values, checking data in
set_value and both left and right in add_values before launching their kernels.
Preserve the existing launch and CUDA error checks.
---
Nitpick comments:
In `@cpp/libcudf_streaming/src/detail/approx_distinct_count.cu`:
- Around line 15-22: Replace the raw __global__ declarations for
set_value_kernel and add_values_kernel with CUDF_KERNEL, and include the header
that directly declares CUDF_KERNEL.
In `@cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp`:
- Around line 117-123: Extend the test around sampled.front() to release the
forwarded table_chunk payload and assert the original table’s row count, column
count, and values, while retaining the existing sequence_number check and
estimate assertions.
- Around line 21-23: Add the direct standard-library headers <cstddef>,
<stdexcept>, and <utility> to support the file’s uses of std::size_t,
std::invalid_argument, and std::move.
🪄 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: e90df714-4711-4a45-9921-ec8f915e48f0
📒 Files selected for processing (16)
cpp/libcudf_streaming/CMakeLists.txtcpp/libcudf_streaming/include/cudf_streaming/approx_distinct_count.hppcpp/libcudf_streaming/include/cudf_streaming/detail/approx_distinct_count.hppcpp/libcudf_streaming/src/approx_distinct_count.cppcpp/libcudf_streaming/src/detail/approx_distinct_count.cucpp/libcudf_streaming/tests/CMakeLists.txtcpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpppython/cudf_polars/cudf_polars/streaming/actor_graph/join.pypython/cudf_polars/cudf_polars/streaming/actor_graph/utils.pypython/cudf_streaming/CMakeLists.txtpython/cudf_streaming/cudf_streaming/__init__.pxdpython/cudf_streaming/cudf_streaming/__init__.pypython/cudf_streaming/cudf_streaming/approx_distinct_count.pxdpython/cudf_streaming/cudf_streaming/approx_distinct_count.pyipython/cudf_streaming/cudf_streaming/approx_distinct_count.pyxpython/cudf_streaming/cudf_streaming/tests/test_approx_distinct_count.py
🚧 Files skipped from review as they are similar to previous changes (12)
- cpp/libcudf_streaming/tests/CMakeLists.txt
- python/cudf_streaming/CMakeLists.txt
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pyi
- python/cudf_streaming/cudf_streaming/init.py
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pyx
- cpp/libcudf_streaming/CMakeLists.txt
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pxd
- python/cudf_polars/cudf_polars/streaming/actor_graph/join.py
- cpp/libcudf_streaming/src/approx_distinct_count.cpp
- python/cudf_streaming/cudf_streaming/tests/test_approx_distinct_count.py
- python/cudf_streaming/cudf_streaming/init.pxd
- python/cudf_polars/cudf_polars/streaming/actor_graph/utils.py
d4b5b5a to
af8eec1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp (1)
127-135: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a valid-boundary precision test.
RejectsInvalidPrecisionchecks that precision 3 and 19 throw. It does not check that the nearest valid boundary values (4 and 18, based on the implied valid range) construct successfully. Add a case that constructscardinality_estimatorwith precision 4 and 18 and expects no throw, to guard the boundary in both directions.♻️ Proposed addition
TEST_F(CardinalityEstimatorTest, RejectsInvalidPrecision) { EXPECT_THROW( cudf_streaming::cardinality_estimator(ctx, GlobalEnvironment->comm_, rapidsmpf::OpID{0}, 3), std::invalid_argument); EXPECT_THROW( cudf_streaming::cardinality_estimator(ctx, GlobalEnvironment->comm_, rapidsmpf::OpID{0}, 19), std::invalid_argument); + EXPECT_NO_THROW( + cudf_streaming::cardinality_estimator(ctx, GlobalEnvironment->comm_, rapidsmpf::OpID{0}, 4)); + EXPECT_NO_THROW( + cudf_streaming::cardinality_estimator(ctx, GlobalEnvironment->comm_, rapidsmpf::OpID{0}, 18)); }🤖 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_approx_distinct_count.cpp` around lines 127 - 135, Extend the CardinalityEstimatorTest coverage around RejectsInvalidPrecision by adding successful construction checks for the valid boundary precisions 4 and 18. Verify cardinality_estimator does not throw for either value while preserving the existing invalid-precision assertions for 3 and 19.
🤖 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_approx_distinct_count.cpp`:
- Around line 127-135: Extend the CardinalityEstimatorTest coverage around
RejectsInvalidPrecision by adding successful construction checks for the valid
boundary precisions 4 and 18. Verify cardinality_estimator does not throw for
either value while preserving the existing invalid-precision assertions for 3
and 19.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 33a5393c-87a0-406f-be8b-6544b4339da4
📒 Files selected for processing (16)
cpp/libcudf_streaming/CMakeLists.txtcpp/libcudf_streaming/include/cudf_streaming/approx_distinct_count.hppcpp/libcudf_streaming/include/cudf_streaming/detail/approx_distinct_count.hppcpp/libcudf_streaming/src/approx_distinct_count.cppcpp/libcudf_streaming/src/detail/approx_distinct_count.cucpp/libcudf_streaming/tests/CMakeLists.txtcpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpppython/cudf_polars/cudf_polars/streaming/actor_graph/join.pypython/cudf_polars/cudf_polars/streaming/actor_graph/utils.pypython/cudf_streaming/CMakeLists.txtpython/cudf_streaming/cudf_streaming/__init__.pxdpython/cudf_streaming/cudf_streaming/__init__.pypython/cudf_streaming/cudf_streaming/approx_distinct_count.pxdpython/cudf_streaming/cudf_streaming/approx_distinct_count.pyipython/cudf_streaming/cudf_streaming/approx_distinct_count.pyxpython/cudf_streaming/cudf_streaming/tests/test_approx_distinct_count.py
🚧 Files skipped from review as they are similar to previous changes (15)
- cpp/libcudf_streaming/CMakeLists.txt
- python/cudf_streaming/cudf_streaming/init.py
- python/cudf_streaming/cudf_streaming/init.pxd
- cpp/libcudf_streaming/tests/CMakeLists.txt
- python/cudf_streaming/CMakeLists.txt
- cpp/libcudf_streaming/include/cudf_streaming/detail/approx_distinct_count.hpp
- python/cudf_polars/cudf_polars/streaming/actor_graph/join.py
- python/cudf_streaming/cudf_streaming/tests/test_approx_distinct_count.py
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pyi
- cpp/libcudf_streaming/include/cudf_streaming/approx_distinct_count.hpp
- cpp/libcudf_streaming/src/detail/approx_distinct_count.cu
- python/cudf_polars/cudf_polars/streaming/actor_graph/utils.py
- cpp/libcudf_streaming/src/approx_distinct_count.cpp
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pxd
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pyx
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class ChunkSampler: |
There was a problem hiding this comment.
Thoughts on adding tests dedicated to this class? Mostly, I'm worried about the is_complete and how that interacts with the statistics we compute. The code looks correct, but I'd be more comfortable with some unit tests.
I don't see existing tests for TableSizeStats, though I might have missed them.
There was a problem hiding this comment.
Mostly, I'm worried about the is_complete and how that interacts with the statistics we compute.
I don't know what you mean here.
There was a problem hiding this comment.
I am generally somewhat leery about adding tests for internals like this. Since the object is not part of the public API.
There was a problem hiding this comment.
https://github.com/rapidsai/cudf/pull/23522/changes#diff-176746e532fa012a79d93db9180e0c4f09ac148678419340b8398a5ab6671641R1148-R1154 is where the value we return for total_size and total_rows depends on whether our sample covered all the messages.
There was a problem hiding this comment.
I'm not sure what is actionable here.
af8eec1 to
95d95c4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp (3)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct headers for standard-library symbols.
This file uses
std::size_t,std::move, andstd::invalid_argument, but it relies on transitive includes. Add<cstddef>,<stdexcept>, and<utility>directly.As per coding guidelines, include headers directly for every used symbol without unused or incorrectly styled includes.
Proposed fix
+#include <cstddef> `#include` <cstdint> `#include` <memory> +#include <stdexcept> +#include <utility> `#include` <vector>🤖 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_approx_distinct_count.cpp` around lines 21 - 23, Update the include list in test_approx_distinct_count.cpp to directly add <cstddef> for std::size_t, <stdexcept> for std::invalid_argument, and <utility> for std::move, while retaining only correctly styled headers required by the file.Source: Coding guidelines
118-124: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the sampled table contents.
The test checks only the sampled message count and sequence number. It does not verify the selected column, row values, or forwarded table. Extract the sampled
cudf_streaming::table_chunkand assert the shape and values required by the channel contract.🤖 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_approx_distinct_count.cpp` around lines 118 - 124, Extend the sampled-message assertions in the test around sampled.front() to extract its cudf_streaming::table_chunk and validate the channel contract: confirm the selected column, table shape, row values, and forwarded table contents in addition to the existing count and sequence-number checks. Preserve the existing cardinality estimate assertions.
31-35: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse rank-specific values to test the global union.
Every rank inserts the same values. A reducer that skips cross-rank sketch merging can still produce the expected distinct count. Build disjoint or partially overlapping ranges using each rank's identity, then assert the resulting global union size separately from the global row count.
🤖 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_approx_distinct_count.cpp` around lines 31 - 35, Update the test data setup around values and dups to incorporate each rank’s identity, creating disjoint or partially overlapping value ranges across ranks so incorrect cross-rank sketch merging cannot pass. Assert the global distinct-union size independently from the global row count, preserving the existing repetition behavior.
🤖 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_polars/cudf_polars/streaming/actor_graph/utils.py`:
- Around line 1147-1159: Update the sampling logic near the `sample_count`
calculation to set `is_complete` when `sample_count` equals the expected local
chunk count (`self.ch_in_chunk_count`), including the `max_chunks` case where no
`None` sentinel is received. Ensure the updated completion state is used by the
existing size, row, total_chunks, and `TableSizeStats` calculations.
---
Nitpick comments:
In `@cpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpp`:
- Around line 21-23: Update the include list in test_approx_distinct_count.cpp
to directly add <cstddef> for std::size_t, <stdexcept> for
std::invalid_argument, and <utility> for std::move, while retaining only
correctly styled headers required by the file.
- Around line 118-124: Extend the sampled-message assertions in the test around
sampled.front() to extract its cudf_streaming::table_chunk and validate the
channel contract: confirm the selected column, table shape, row values, and
forwarded table contents in addition to the existing count and sequence-number
checks. Preserve the existing cardinality estimate assertions.
- Around line 31-35: Update the test data setup around values and dups to
incorporate each rank’s identity, creating disjoint or partially overlapping
value ranges across ranks so incorrect cross-rank sketch merging cannot pass.
Assert the global distinct-union size independently from the global row count,
preserving the existing repetition behavior.
🪄 Autofix
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: 426e1b21-5a79-450b-aa89-38714bb49fc4
📒 Files selected for processing (16)
cpp/libcudf_streaming/CMakeLists.txtcpp/libcudf_streaming/include/cudf_streaming/approx_distinct_count.hppcpp/libcudf_streaming/include/cudf_streaming/detail/approx_distinct_count.hppcpp/libcudf_streaming/src/approx_distinct_count.cppcpp/libcudf_streaming/src/detail/approx_distinct_count.cucpp/libcudf_streaming/tests/CMakeLists.txtcpp/libcudf_streaming/tests/streaming/test_approx_distinct_count.cpppython/cudf_polars/cudf_polars/streaming/actor_graph/join.pypython/cudf_polars/cudf_polars/streaming/actor_graph/utils.pypython/cudf_streaming/CMakeLists.txtpython/cudf_streaming/cudf_streaming/__init__.pxdpython/cudf_streaming/cudf_streaming/__init__.pypython/cudf_streaming/cudf_streaming/approx_distinct_count.pxdpython/cudf_streaming/cudf_streaming/approx_distinct_count.pyipython/cudf_streaming/cudf_streaming/approx_distinct_count.pyxpython/cudf_streaming/cudf_streaming/tests/test_approx_distinct_count.py
🚧 Files skipped from review as they are similar to previous changes (14)
- python/cudf_streaming/cudf_streaming/init.pxd
- cpp/libcudf_streaming/include/cudf_streaming/detail/approx_distinct_count.hpp
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pyi
- cpp/libcudf_streaming/tests/CMakeLists.txt
- python/cudf_streaming/cudf_streaming/init.py
- python/cudf_streaming/CMakeLists.txt
- python/cudf_polars/cudf_polars/streaming/actor_graph/join.py
- cpp/libcudf_streaming/src/detail/approx_distinct_count.cu
- cpp/libcudf_streaming/CMakeLists.txt
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pyx
- cpp/libcudf_streaming/include/cudf_streaming/approx_distinct_count.hpp
- python/cudf_streaming/cudf_streaming/approx_distinct_count.pxd
- python/cudf_streaming/cudf_streaming/tests/test_approx_distinct_count.py
- cpp/libcudf_streaming/src/approx_distinct_count.cpp
|
/merge |
6 participants
Description
We will use this to estimate cardinalities of tables in join prefiltering. Hence to that end also introduce ChunkSampler in cudf-polars streaming to manage the various input/output channels.
Checklist