Roaring Bitmap Filter in CAGRA - #2446
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. |
divyegala
added
feature request
|
/ok to test 1529d8f |
dantegd
left a comment
dantegd
left a comment
There was a problem hiding this comment.
Should we explicitly limit Roaring filters to direct cagra::search for now? I don’t think the query mapping works cleanly with dynamic batching because CAGRA may receive either max_batch_size or the actual batch size. Tiered search also passes the same filter to CAGRA and brute-force partitions with different row domains, and brute force doesn’t support Roaring. Since cagra::merge already rejects this filter type, would it make sense to reject or document these paths too, unless they’re intended to be supported? What do you think?
| tenant_views.push_back(allowlist.view()); | ||
| } | ||
| cuvs::neighbors::filtering::roaring_filter roaring_filter(res, tenant_views); | ||
| auto roaring_result = search(roaring_filter, 2.0f / 3.0f); |
There was a problem hiding this comment.
Could we clarify the stream semantics here? Construction may return while serialization and the device-reference copy are still queued. Using the result on the same stream is safe, but another stream needs an explicit dependency, like the event handoff in the test. The current comment explains how long the input must remain valid, but not when the returned view is safe to use.
There was a problem hiding this comment.
We commit to stream ordered synchronization guarantees on raft::resources. Is that not already true here? If the host returns execution, it is safe to use.
| auto& sample_filter = | ||
| dynamic_cast<const cuvs::neighbors::filtering::roaring_filter&>(sample_filter_ref); | ||
| RAFT_EXPECTS(sample_filter.valid(), "roaring_filter must be initialized before search."); | ||
| RAFT_EXPECTS(sample_filter.num_queries() == static_cast<std::size_t>(queries.extent(0)), |
There was a problem hiding this comment.
Do we expect Roaring filters to work through dynamic batching as well? Dynamic batching may call CAGRA with either max_batch_size or the actual batch size, so this equality can fail depending on the dispatch mode. If that path isn’t supported yet, should we document it or reject the filter earlier?
| { | ||
| auto minimum_cardinality = dataset_rows; | ||
| for (auto const& allowlist : allowlists) { | ||
| minimum_cardinality = std::min(minimum_cardinality, allowlist.cardinality()); |
There was a problem hiding this comment.
Is using the minimum cardinality intentional here? One very small or empty allowlist makes the batch-wide filtering rate approach 0.999, which can substantially increase the MULTI_CTA work for every query in the batch. Would it be worth adding coverage for mixed-cardinality batches or documenting this tradeoff?
There was a problem hiding this comment.
This is documented, I ran into this problem myself. The right way to do this for users it to set search_params::filtering_rate so that it is a CAGRA search specific hint, and users can tradeoff between latency and recall.
3 participants
This PR uses cuco's Roaring Bitmap filter implementation to work with CAGRA search. Currently, it only supports single allowlist construction.
Benchmarks on GIST 1M, measuring single-batch search latency + filter construction time:
Filter sizes: