Skip to content

Add approx_distinct_count - #20735

Merged
rapids-bot[bot] merged 54 commits into
NVIDIA:mainfrom
PointKernel:approx-distinct-count
Jan 14, 2026
Merged

Add approx_distinct_count#20735
rapids-bot[bot] merged 54 commits into
NVIDIA:mainfrom
PointKernel:approx-distinct-count

Conversation

@PointKernel

@PointKernel PointKernel commented Nov 26, 2025

Copy link
Copy Markdown
Member

Description

Closes #20774

This PR adds an approx_distinct_count class which uses Hyperloglog++ under the hood to provide approximate distinct count estimation.

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 Nov 26, 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 libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Nov 26, 2025
Comment thread cpp/src/stream_compaction/approx_distinct_count.cu Outdated
PointKernel added a commit to NVIDIA/cuCollections that referenced this pull request Dec 10, 2025
…unction (#779)

While working on NVIDIA/cudf#20735, we
observed that the HLL ref was not copyable. This PR resolves the issue
by converting the `register_mask` data member into a member function,
which makes the HLL reference implicitly copyable as intended.
Comment thread cpp/src/stream_compaction/approx_distinct_count.cu Outdated
@PointKernel
PointKernel requested a review from ttnghia January 12, 2026 19:44
@ttnghia

ttnghia commented Jan 12, 2026

Copy link
Copy Markdown
Contributor

As discussed, should the class approx_distinct_count be a template class taking a Hasher template parameter so it allows the callers to override by their own hasher?

@GregoryKimball GregoryKimball moved this to Burndown in libcudf Jan 12, 2026
@PointKernel

Copy link
Copy Markdown
Member Author

As discussed, should the class approx_distinct_count be a template class taking a Hasher template parameter so it allows the callers to override by their own hasher?

@ttnghia There is a design question to clarify. We generally do not expose public device APIs or templated APIs in libcudf. Making the API templated also means we can’t separate declarations and definitions, which effectively forces it into a .cuh header.

This PR was primarily motivated by the Velox use case. After checking with the team, cuDF operators are currently integrated into Velox via host-side APIs rather than device APIs. Given that approx_distinct_count is not Spark-compatible at the moment, would it make sense to address the Spark integration as a separate task?

@ttnghia

ttnghia commented Jan 12, 2026

Copy link
Copy Markdown
Contributor

How about only implement the detailed class approx_distinct_count as a template class, keeping the class declared in the public header intact? We (spark-rapids-jni) can pull in the detail header and use it directly.

namespace cudf {
namespace detail {

// This is for overriding by a customized hasher.
template<typename Hasher>
class approx_distinct_count_base {};

class approx_distinct_count: public approx_distinct_count_base {};

@ttnghia

ttnghia commented Jan 12, 2026

Copy link
Copy Markdown
Contributor

FYI I'm fine with defer further changes to the next PR merging into the next release. Just want to make clear about the direction that we need to move onto.

However, if the suggested changes only take a few lines of code then it would be great to have here 😄

@PointKernel

Copy link
Copy Markdown
Member Author

How about only implement the detailed class approx_distinct_count as a template class, keeping the class declared in the public header intact? We (spark-rapids-jni) can pull in the detail header and use it directly.

namespace cudf {
namespace detail {

// This is for overriding by a customized hasher.
template<typename Hasher>
class approx_distinct_count_base {};

class approx_distinct_count: public approx_distinct_count_base {};

Sure, that’s what I wanted to propose as well. We’re already following the same pattern for hash_join.

*
* This range represents a practical engineering compromise from HLL++ and is widely
* adopted by systems such as Apache Spark. The default of 12 aligns with Spark's
* configuration and is the largest precision that fits efficiently in GPU shared memory,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This isn't fully correct. p=12 results in a 16KB sketch which is just a small fraction of the available shared memory. cuco's implementation uses dynamic shared memory, and can thus go beyond 200KB on B200 for example. p=13 (32KB) safely fits into static shared memory on older architectures that don't have extended dynamic shared memory capacity.

@PointKernel PointKernel Jan 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For reference: https://github.com/NVIDIA/spark-rapids-jni/blob/ed68339b01ead5d5f81163bf0431b542b786ec67/src/main/cpp/src/hyper_log_log_plus_plus.cu#L46-L51

My guess is that the tests were run on a the lowest-performance card needed by spark or they need to support multi-stream executions on a single card.

* @param stream CUDA stream used for device memory operations and kernel launches
*/
approx_distinct_count(cuda::std::span<cuda::std::byte> sketch_span,
std::int32_t precision,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

precision is a parameter specific to the HLL++ implementation. Ideally the interface is agnostic to the algorithm under the hood. We could replace it with some more general concept like standard deviation and then select the corresponding lower bound precision value for the implementation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed. See #20735 (comment)

@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.

Some small comments, but generally looks good.

Comment thread cpp/include/cudf/reduction/approx_distinct_count.hpp Outdated
Comment thread cpp/src/reductions/approx_distinct_count.cu
Comment thread cpp/src/reductions/approx_distinct_count.cu
Comment thread cpp/src/reductions/approx_distinct_count.cu
Comment thread cpp/src/reductions/approx_distinct_count.cu
@PointKernel

Copy link
Copy Markdown
Member Author

/merge

@rapids-bot
rapids-bot Bot merged commit 080c04a into NVIDIA:main Jan 14, 2026
284 of 286 checks passed
@PointKernel
PointKernel deleted the approx-distinct-count branch January 14, 2026 17:25
@vuule vuule moved this from Burndown to Landed in libcudf Jan 14, 2026
@GregoryKimball GregoryKimball removed this from libcudf Feb 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 - Ready for Review Ready for review by team CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEA] Add object-oriented support for approx_distinct_count

9 participants