Refactoring: Replace BloomFilter with combination of CBitmap, CRoaring and CBloomfilter for index pre-filtering - #24787
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Pull request overview
Refactors index pre-filtering to replace the previous BloomFilter-only approach with a unified doc_id membership filter abstraction backed by dense CBitmap, CRoaring (roaring64), or CBloomFilter, and wires this through runtime-filter build/probe paths for IVFFLAT and fulltext.
Changes:
- Introduces
pkg/common/docfilter(Build/New + exactness + C-bridge) and adds CRoaring/cbitmap C wrappers to support fast exact filtering for integer PKs. - Reworks runtime filter plumbing (plan/pipeline protos, message types, compile/runtime propagation) to transport tagged membership-filter payloads instead of bloom-only bytes.
- Adds/updates unit + BVT tests to cover cbitmap/CRoaring/bloom routing and correctness across ivfflat + fulltext.
Reviewed changes
Copilot reviewed 61 out of 67 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| thirdparties/Makefile | Adds CRoaring third-party build/install for libroaring.a + roaring.h. |
| test/distributed/cases/vector/vector_ivf_membership.sql | New BVT verifying ivfflat pre mode membership-filter routing across PK types/ranges. |
| test/distributed/cases/vector/vector_ivf_membership.result | Expected output for ivfflat membership-filter BVT. |
| test/distributed/cases/fulltext/fulltext_membership.sql | New BVT verifying fulltext membership-filter pushdown across PK types/ranges. |
| test/distributed/cases/fulltext/fulltext_membership.result | Expected output for fulltext membership-filter BVT. |
| proto/plan.proto | Renames use_bloom_filter → use_membership_filter in RuntimeFilterSpec. |
| proto/pipeline.proto | Renames bloom_filter → membership_filter bytes on pipeline.Source. |
| pkg/vm/message/runtimeFilterMsg.go | Renames runtime-filter type constant to UNIQUEJOINKEYS. |
| pkg/vm/engine/types.go | Introduces engine.MembershipFilter + updates FilterHint fields. |
| pkg/vm/engine/readutil/pk_filter.go | Switches PK filter wrapping from CBloomFilter to engine.MembershipFilter. |
| pkg/vm/engine/disttae/txn_table.go | Rebuilds membership filter from tagged bytes; adds share/free error-path hygiene and type-assignability assertion. |
| pkg/vectorindex/usearchex/search.go | Replaces filtered search implementation to accept docfilter.MembershipFilter and dispatch via CHandle/CKind. |
| pkg/vectorindex/usearchex/search_test.go | Updates/extends tests for membership-filter (cbitmap/CRoaring/bloom) behavior. |
| pkg/vectorindex/sqlexec/sqlexec.go | Propagates membership-filter bytes via context keys for internal SQL execution. |
| pkg/vectorindex/sqlexec/literal_test.go | Adds regression test to ensure BIT PK literals are rendered as safe hex (x'..'). |
| pkg/vectorindex/sqlexec/bloomfilter.go | Renames wait helper to WaitUniqueJoinKeys; BIT literal emission now uses hex. |
| pkg/vectorindex/sqlexec/bloomfilter_test.go | Updates wait/AppendVectorSQLLiteral tests for membership-filter + hex BIT literals. |
| pkg/vectorindex/ivfflat/search.go | Removes centroid bloom preload/merge path; builds exact membership filter (or exact pk IN) from unique join keys. |
| pkg/vectorindex/ivfflat/getbloomfilter_test.go | New tests covering membership-filter payload build vs exact pk IN threshold. |
| pkg/sql/plan/apply_indices_ivfflat.go | Planner emits runtime filter specs using UseMembershipFilter. |
| pkg/sql/plan/apply_indices_fulltext.go | Planner emits runtime filter specs using UseMembershipFilter. |
| pkg/sql/compile/types.go | Compile-time Source now carries MembershipFilterBytes instead of BloomFilter. |
| pkg/sql/compile/scope.go | Passes membership-filter bytes into engine.FilterHint for ivf/fulltext index scans. |
| pkg/sql/compile/scope_test.go | Updates tests to assert membership-filter propagation into FilterHint. |
| pkg/sql/compile/remoterun.go | Serializes/deserializes membership-filter bytes through pipeline Source. |
| pkg/sql/compile/remoterun_bf_test.go | Updates remote-run propagation tests for membership-filter bytes. |
| pkg/sql/compile/bloomfilter_cover_test.go | Updates coverage tests for membership-filter branches. |
| pkg/sql/colexec/table_function/ivfpq_search_test.go | Minor import/order formatting adjustments. |
| pkg/sql/colexec/table_function/ivfpq_create_test.go | Minor formatting adjustments in tests. |
| pkg/sql/colexec/table_function/fulltext.go | Switches fulltext pushdown from bloom build to tagged docfilter.Build membership filter payload. |
| pkg/sql/colexec/table_function/fulltext_test.go | Updates test to assert membership-filter propagation into fast-path SQL. |
| pkg/sql/colexec/hashbuild/build.go | Runtime-filter build side now sends UNIQUEJOINKEYS when membership-filter requested. |
| pkg/defines/type.go | Adds context keys IvfMembershipFilter / FulltextMembershipFilter. |
| pkg/cuvs/search_async_batch_test.go | Minor edits around call-site argument label comments in async-batch tests. |
| pkg/cuvs/ivf_pq.go | Formatting/alignment changes. |
| pkg/cuvs/ivf_flat.go | Formatting/alignment changes. |
| pkg/cuvs/cagra.go | Formatting/alignment changes. |
| pkg/common/docfilter/filter.go | New public membership-filter API (Build/New) + interfaces (probe + C bridge). |
| pkg/common/docfilter/filter_test.go | New tests for Build/New routing, exactness, Share, and C bridge contract. |
| pkg/common/docfilter/extreme_test.go | New guards for extreme integer PK values and cbitmap feasibility boundaries. |
| pkg/common/docfilter/docfilter.go | New package docs + type routing helpers and raw integer decode helper. |
| pkg/common/docfilter/docfilter_test.go | Tests for SupportsBitset and shared helpers. |
| pkg/common/docfilter/croaring.go | Implements CRoaring-backed exact membership filter + serialization/refcount. |
| pkg/common/docfilter/croaring_test.go | Tests for CRoaring filter correctness and share behavior. |
| pkg/common/docfilter/corrupt_test.go | Regression test ensuring corrupt bloom payloads are rejected safely. |
| pkg/common/docfilter/cbitmap.go | Implements dense cbitmap filter with optional base-offset and strict status handling. |
| pkg/common/docfilter/cbitmap_test.go | Tests for cbitmap behavior, feasibility gating, and negative int32 handling with offset. |
| pkg/common/docfilter/bench_test.go | Benchmarks for build/probe across bloom/cbitmap/croaring and offset/run-opt variants. |
| pkg/common/bloomfilter/exact_test.go | Pins CBloomFilter.Exact() semantics for the new interface contract. |
| pkg/common/bloomfilter/cbloomfilter.go | Adds Exact() bool to CBloomFilter to satisfy consumer membership interface. |
| go.mod | Minor require ordering change (no semantic change). |
| cgo/usearchex.h | Replaces bloom/bitmap filtered search API with membership-filter kind dispatch. |
| cgo/usearchex.c | Implements membership-filter predicate dispatch across bloom/cbitmap/croaring. |
| cgo/test/test_croaring.c | Adds CRoaring C-level tests (build/probe/serialize/null safety). |
| cgo/test/test_cbitmap.c | Adds cbitmap C-level tests including offset layout and status codes. |
| cgo/test/Makefile | Links CRoaring and builds new cbitmap/croaring tests. |
| cgo/Makefile | Switches C standard to C11; links -lroaring; adds croaring/cbitmap objects. |
| cgo/lib.go | Switches cgo CFLAGS to C11; links -lroaring in cgo LDFLAGS. |
| cgo/croaring.h | New CRoaring wrapper header for fixed-vector add/test + portable serialization. |
| cgo/croaring.c | New CRoaring wrapper implementation with chunked bulk add + safe deserialize. |
| cgo/cbitmap.h | New cbitmap wrapper header with explicit build status codes and (de)serialization. |
| cgo/cbitmap.c | New cbitmap wrapper implementation with optional base-offset and host-endian serialization guard. |
| cgo/bloom.c | Hardens bloom unmarshal to reject invalid/corrupt payload sizes and nbits. |
|
Found a few issues that should be fixed before merge:
|
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Requesting changes based on the unresolved issues in my previous comment: the IVF exact-PK path still lacks ORDER BY vec_dist LIMIT, the deserialized runtime key vector is still not freed, and mo_cbitmap_deserialize still accepts overflowed/corrupt nbits headers as valid filters. These can affect query correctness or leak memory, so they should be fixed before merge.
Details: #24787 (comment)
Two review findings in the small-key ExactPkFilter path of ivfflat search: 1. The "pk IN (...)" query was built without ORDER BY vec_dist LIMIT, so it returned up to exactPkFilterThreshold candidates in arbitrary scan order and count. The result processing does not re-sort or truncate (it returns rows in SQL order), and the centroid path relies on the SQL's ORDER BY / LIMIT for top-k, so the exact-PK path broke IVF top-k semantics. Add the same ORDER BY vec_dist LIMIT %d. 2. keyvec (deserialized from the runtime-filter bytes) was never freed, leaking on every build. defer keyvec.Free(sqlproc.Proc.Mp()), matching the fulltext path; the filter builders only read keyvec and return copies, so freeing it on return is safe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mo_cbitmap_deserialize sized the read with bitmap_size(nbits)=(nbits+63)>>6 computed from the untrusted header nbits, which overflows for large nbits (e.g. MaxUint64 -> nwords=0). A forged 16-byte payload was then accepted as a valid filter that crashes / matches nothing on probe, silently dropping rows if a transported payload is corrupted. Derive the word count from the actual payload length and require nbits to be exactly consistent (ceil(nbits/64) == nwords, range-checked without the +63 overflow), rejecting payloads whose length is not a whole number of words or whose nbits does not match. Add TestCorruptCbitmapPayloadRejected (huge nbits, inconsistent nbits, non-word-aligned, truncated), mirroring the Bloom corrupt test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mo_cbitmap_deserialize sized the read with bitmap_size(nbits)=(nbits+63)>>6 computed from the untrusted header nbits, which overflows for large nbits (e.g. MaxUint64 -> nwords=0). A forged 16-byte payload was then accepted as a valid filter that crashes / matches nothing on probe, silently dropping rows if a transported payload is corrupted. Derive the word count from the actual payload length and require nbits to be exactly consistent (ceil(nbits/64) == nwords, range-checked without the +63 overflow), rejecting payloads whose length is not a whole number of words or whose nbits does not match. Add TestCorruptCbitmapPayloadRejected (huge nbits, inconsistent nbits, non-word-aligned, truncated), mirroring the Bloom corrupt test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The small-key ExactPkFilter path built "pk IN (...)" without ORDER BY vec_dist LIMIT, so it returned up to exactPkFilterThreshold candidates in arbitrary scan order and count. The result processing returns rows in SQL order without re-sorting or truncating, and the centroid path relies on the SQL's ORDER BY / LIMIT for top-k, so the exact-PK path broke IVF top-k semantics. Add the same ORDER BY vec_dist LIMIT %d. Also document why the deserialized keyvec is intentionally not freed: UnmarshalBinary aliases the input bytes (cantFreeData/cantFreeArea set), so it owns no mpool memory (GC reclaims the struct and aliased bytes); calling Free(mp) would be a no-op for this zero-copy path and would tie release to a specific mpool, a cross-pool hazard if the path ever became owning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
UnmarshalBinary aliases the input bytes (sets cantFreeData/cantFreeArea), so the deserialized keyvec owns no mpool memory — the struct and aliased bytes are reclaimed by GC. The defer keyvec.Free(proc.Mp()) was a no-op for this zero-copy path and tied release to a specific mpool, a cross-pool free hazard if the deserialization ever became owning. Drop it, matching the IVF path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
For IVF per-centroid bloom preload removed entirely and The old code narrowed keys by centroid membership before building the bloom filter. Running SQL is to get the centroids is I/O bound especially when data is on disk. It will be much slower than building bloom filter. Remove the code make sense. |
An earlier change added "ORDER BY vec_dist LIMIT k" to the exact-PK (mode=pre, small key set) entries query. That regressed vector_ivf_mode.sql: LIMIT k returned k-1 rows. Unlike the centroid path -- whose scanned entries are all valid indexed vectors, so its ordered top-k IS the answer -- the exact-PK path filters by "pk IN (candidates)" from the relational predicate and returns the full small candidate set for the mode=pre consumer to rank and LIMIT. A row the index orders ahead of a wanted row but the consumer discards consumes a LIMIT slot, dropping a real result. Restore the original (no ORDER BY / LIMIT) and document why ranking/truncation belongs to the consumer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed from three angles:
- Correctness and logic: no blocking defects found.
- Performance and resource impact: changes are acceptable and risks are covered.
- Compatibility and maintainability: behavior and test coverage look sufficient for merge.
… post-filtering)
The exact-PK (mode=pre, small key set) entries query had "ORDER BY vec_dist
LIMIT k" added, which regressed vector_ivf_mode.sql.
Root cause (reproduced in pure SQL + EXPLAIN against the entries table): adding
"ORDER BY vec_dist LIMIT k" makes the planner push the sort+limit INTO the
entries Table Scan -- EXPLAIN shows "Index Reader Param: Sort Key ... Limit: k"
on the scan node. That applies the LIMIT *before* the "pk IN (...)" / prefix_eq
filter, turning our intended pre-filter into a POST-filter: it takes the global
top-k by distance over ALL entries, then keeps only the candidates, so fewer
than k matching rows survive. E.g. candidate set {1,2,7} with LIMIT 3 returns
{1,2} -- the global top-3 by distance is {1,2,5}, and pk 7 (rank #7) never makes
the cut. Without ORDER BY/LIMIT the scan stays a plain filtered read (no Index
Reader Param) returning the full candidate set, and the downstream
Node_SORT + LIMIT k does the ranking and truncation.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…search table scan
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Rechecked the latest head and the previous blocking points are resolved or clarified.
UnmarshalBinaryis zero-copy and marks the vector data/area as non-owned, so not callingFree(mp)here is appropriate.- The IVF exact-PK query intentionally avoids inner
ORDER BY vec_dist LIMITto prevent planner/index-reader pushdown from applying limit before the PK filter; returning the small candidate set and letting the outer sort/limit rank it is reasonable. - The cbitmap corrupt-payload issue is fixed with overflow-safe length/nbits validation and regression coverage.
LGTM.
Merge Queue Status
This pull request spent 1 hour 59 minutes 34 seconds in the queue, including 1 hour 2 minutes 44 seconds running CI. Required conditions to merge
|
What type of PR is this?
Which issue(s) this PR fixes:
issue #24782
What this PR does / why we need it:
CBitmap is the fastest for both build and search time but it require bigger memory space and integer id. CRoaring is comparable to CBloomfilter but it is exact and smaller memory footprint than bloomfilter.
CBloomfilter is the slowest but it support non-integer primary key
when the data size is less than 16M (max ID/count) and key is integer, choose cbitmap.
Otherwise use CRoaring.
For non-integer primary key, use CBloomFilter.