enhance: support nullable external id mapping by marcelo-cjl · Pull Request #1673 · zilliztech/knowhere

marcelo-cjl · 2026-06-10T06:18:52Z

issue: #1687

related: zilliztech/cardinal#859
related: zilliztech/cardinal#860

Summary

Add snapshot-backed nullable id mapping in Knowhere so compact raw/index storage can expose stable public out ids without segcore keeping another sealed mapping copy.
Define the IndexNode boundary contract: search-like results are mapped to public out ids once before returning to Knowhere callers, while backend/core layers keep their local relayout ids internally.
Thread mapped bitset/count handling through brute-force search, iterators, data-view refinement, dense/sparse/vector retrieval, and emb-list TokenANN paths.
Preserve backend-specific mapping ownership for Faiss, HNSW, DiskANN/AiSAQ, SVS, sparse, CUVS, and Cardinal integrations without adding hot-path result remapping in the public Index wrapper.
Add nullable id-map coverage for search/range/iterator/retrieve/reload cases, including TokenANN GetEmbListByIds and unsupported raw retrieve behavior.

Verification

make pre-commit
cmake --build build/Release --target knowhere_tests -j 8
build/Release/tests/ut/knowhere_tests "Test_AiSAQ_Params"
build/Release/tests/ut/knowhere_tests "Nullable TokenANN GetEmbListByIds uses logical ids"

mergify · 2026-06-10T06:19:31Z

@marcelo-cjl 🔍 Important: PR Classification Needed!

For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:

If you're fixing a bug, label it as kind/bug.
For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
Adjusting APIs or changing functionality? Go with kind/feature.

For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”.

Thanks for your efforts and contribution to the community!.

foxspy · 2026-06-11T16:27:09Z

I think the ID ownership boundary here needs to be made explicit.

IndexNodeWithDataViewRefiner is the public IndexNode boundary, so public APIs should consume/return external labels. But DataViewIndexFlat is an internal refiner and its subset-distance path indexes directly into the data view, so SearchWithIds() / RangeSearchWithIds() / ComputeDistanceSubset() should operate on internal row offsets only.

Right now the refiner path mixes the two semantics: the ids are used directly by ComputeDistanceSubset() as internal offsets, while the cosine norm path later calls external_id_map_.ToInternalIds() as if the same ids were external labels. For nullable or non-identity mappings this can use the wrong norm id, wrong data-view row, or return labels with inconsistent mapping semantics.

Can we keep the refiner contract internal-only and move all external/internal conversion to IndexNodeWithDataViewRefiner? In other words:

map external filters before calling the base index;
keep base/refiner candidate ids as internal offsets;
map final result labels to external ids exactly once before returning from the public IndexNode API.

foxspy · 2026-06-11T16:27:11Z

The ExternalIdMap wire format looks incomplete for the runtime state it now represents.

Serialize() currently persists only external_count_ and internal_to_external_ids_, but the map can also carry external_id_offset_ and internal_id_to_emb_list_id_. After save/load, those fields are reset by Clear() and never restored, so indexes relying on identity + non-zero offset or emb-list/DVR mapping can silently return different labels after reload.

Can we persist all required source-of-truth fields and rebuild only derived state during Deserialize()? For example:

persist external_id_offset_;
persist internal_id_to_emb_list_id_ when present;
rebuild external_to_internal_ids_ and valid_bitmap_ from the persisted data;
add save/load regression tests for non-zero offset, nullable mapping, same-size non-identity mapping, and emb-list mapping.

foxspy · 2026-06-11T16:32:01Z

GetExternalIdMap(true) looks expensive to call on the BruteForce search hot path.

DataSet::GetExternalIdMap() constructs a fresh ExternalIdMap every time. When an explicit internal-to-external mapping is present, SetInternalToExternalIds() rebuilds the derived state, including the inverse unordered_map and the valid bitmap. The BruteForce paths call this per request (SearchWithBuf, range/search variants, and iterator setup), so the request now pays an extra O(nb) map/bitmap construction cost before the actual brute-force computation starts.

This may be acceptable for very small datasets, but for large base_dataset inputs it is a material latency and allocation regression, especially because it is independent of nq/topk and repeated for every call.

Can we avoid rebuilding this per search? A few options:

cache the constructed ExternalIdMap inside DataSet and invalidate it only when the source ids/bitmap/offset changes;
pass a stable ExternalIdMap snapshot/reference with the dataset;
or provide benchmark data showing the per-request rebuild cost is negligible for the intended BruteForce workloads.

foxspy · 2026-06-11T16:32:04Z

EnsureBitsetCount() adds another full scan on filtered search paths.

For every non-empty bitset, Index::Search() / RangeSearch() / AnnIterator() call EnsureBitsetCount() before dispatching to the index node. In the normal path this calls bitset.count_filtered_bits(valid_bitmap), which scans the full bitset byte range and optionally ANDs with the valid bitmap. In the emb-list path, CountEmbListBitset() loops over every internal doc id and calls bitset.test().

So filtered search now has an extra O(bitset_size) or O(num_emb_lists) pass per request before the actual index search. That could be significant for large collections and high-QPS filtered workloads, even when nq/topk are small.

Can we either avoid this full count on the hot path or quantify the overhead? Possible approaches:

cache the mapped filtered count together with the mapped bitset;
compute the count lazily only for index implementations that actually need it;
preserve an "unknown count" state instead of forcing an exact count everywhere;
add benchmark coverage for large bitsets / nullable maps / emb-list filters to show the overhead is acceptable.

sre-ci-robot · 2026-06-24T00:07:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marcelo-cjl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [marcelo-cjl]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

marcelo-cjl · 2026-06-25T02:41:50Z

@foxspy Re DataView refiner id boundary: current head keeps DataViewIndexFlat internal-only. SearchWithIds() / RangeSearchWithIds() / CalcDistByIDs() consume internal row offsets; external-to-internal conversion is only done in IndexNodeWithDataViewRefiner, and final results are mapped to out ids once before returning. Tests cover selected-id APIs and cosine norm ids.

marcelo-cjl · 2026-06-25T02:41:51Z

@foxspy Re IdMap wire format: current design changed here. IdMap is runtime-only and is not serialized into the index BinarySet. The caller persists valid-row state externally and sets the runtime map before build/load. Tests assert EXTERNAL_ID_MAP is not written and reload uses the runtime bitmap/map.

marcelo-cjl · 2026-06-25T02:41:52Z

@foxspy Re BruteForce hot path cost: outdated after the refactor. GetExternalIdMap() is gone, and BF does not construct an id map per request. It only applies the existing BitsetView offset/count for the searched range.

marcelo-cjl · 2026-06-25T02:41:53Z

@foxspy Re bitset count scan: this is not an extra scan before another scan. Exact-count indexes already need bitset.count() / filter_ratio() later (HNSW BF fallback/kAlpha, IVF max_codes, etc.). PrepareBitset() computes that once up front; downstream reads the stored count. Indexes that do not need exact count skip it.

alexanderguzhva · 2026-06-30T20:50:59Z

@marcelo-cjl is this PR ready to be reviewed?

Signed-off-by: marcelo-cjl <marcelo.chen@zilliz.com>

alexanderguzhva · 2026-07-14T20:55:44Z

    size_t num_bits_ = 0;
-    size_t num_filtered_out_bits_ = 0;
+    size_t vector_count_ = 0;
+    int64_t filtered_count_ = -1;


please make size_t filtered_count_ = 0; instead. I see no reason of it being int64_t

alexanderguzhva · 2026-07-14T20:56:56Z

    // optional. many indexes will share one bitset, requiring offset to distinguish between them.
    //  like multi-chunk brute-force in /src/common/comp/brute_force.cc, or mv-only in /src/index/hnsw/faiss_hnsw.cc
-    size_t id_offset_ = 0;  // offset of the internal ids
+    int64_t id_offset_ = 0;  // offset of the internal ids


please make size_t id_offset_ = 0; instead. I see no reason of it being int64_t

alexanderguzhva · 2026-07-14T21:02:19Z

+        const auto count_bits = std::min(bit_count, num_bits_ - bit_offset);
+        const auto end_bit = bit_offset + count_bits;
+        size_t bit_pos = bit_offset;
+        size_t count = 0;


please be consistent with names. If we're counting the future filtered_count_ value, then this variable should be named as filtered_count.

alexanderguzhva · 2026-07-14T21:02:31Z

    }

+    void
+    count_filtered_bits(size_t bit_offset, size_t bit_count, const uint8_t* valid_bitmap = nullptr) {


please add a comment on what this function does.

alexanderguzhva · 2026-07-14T21:03:22Z


    void
-    set_id_offset(size_t id_offset) {
+    set_id_offset(int64_t id_offset) {


same comment about int64_t

alexanderguzhva · 2026-07-14T21:31:21Z

+    }
+
+    virtual Status
+    FinalizeIdMap() {


please add a comment for this function about what it does and how to use it properly

alexanderguzhva · 2026-07-14T21:38:43Z

            faiss::cppcontrib::knowhere::HeapBlockResultHandler<CMAX> res(n, distances, labels, k);
            for (auto i = 0; i < n; i++) {
-                futs.emplace_back(search_pool->push([&] {
+                futs.emplace_back(search_pool->push([&, i = i] {


so, you're saying that this DataViewIndexFlat::Search() code has been working incorrectly, because of the missing i=i clause in the futs.emplace_back(search_pool->push([&, i = i] { line.

This is a definite bug that this PR fixes. Same for the similar lines.

alexanderguzhva · 2026-07-14T21:44:47Z

            for (int32_t i = 0; i < ann_k; ++i) {
-                int64_t doc_id = ann_ids[q * ann_k + i];
-                if (doc_id >= 0 && doc_id < num_docs_) {
+                int64_t doc_id = to_in_list_id(ann_ids[q * ann_k + i]);


doc_id < num_docs_ condition is gone

alexanderguzhva · 2026-07-14T21:44:57Z

            for (int32_t i = 0; i < ann_k; ++i) {
-                int64_t doc_id = ann_ids[q * ann_k + i];
-                if (doc_id >= 0 && doc_id < num_docs_) {
+                int64_t doc_id = to_in_list_id(ann_ids[q * ann_k + i]);


alexanderguzhva · 2026-07-14T21:47:01Z

-        return Status::success;
+    void
+    PrepareBitsetForSubIndex(BitsetView& bitset, size_t index_id) const {
+        const auto& out_ids = bitset_out_ids_[index_id];


does this code preserve mv-related logic?

sre-ci-robot requested review from chasingegg and zhengbuqian June 10, 2026 06:18

sre-ci-robot added the approved label Jun 10, 2026

marcelo-cjl mentioned this pull request Jun 10, 2026

feat: support nullable external id mapping #1672

Closed

2 tasks

sre-ci-robot added the size/XXL label Jun 10, 2026

mergify Bot added the dco-passed label Jun 10, 2026

mergify Bot added the do-not-merge/missing-related-issue label Jun 10, 2026

marcelo-cjl force-pushed the codex/nullable-external-id-map branch 4 times, most recently from b708b6b to 8758e14 Compare June 11, 2026 00:54

marcelo-cjl force-pushed the codex/nullable-external-id-map branch 2 times, most recently from 95c9a04 to b7cf82e Compare June 12, 2026 11:47

mergify Bot added the ci-passed label Jun 13, 2026

marcelo-cjl force-pushed the codex/nullable-external-id-map branch from fce23f6 to 285af48 Compare June 13, 2026 02:18

mergify Bot removed the ci-passed label Jun 13, 2026

marcelo-cjl force-pushed the codex/nullable-external-id-map branch from 285af48 to 8cc525e Compare June 13, 2026 02:32

mergify Bot added the ci-passed label Jun 13, 2026

marcelo-cjl mentioned this pull request Jun 14, 2026

enhance: move nullable vector mapping into index layer milvus-io/milvus#50524

Open

6 tasks

marcelo-cjl force-pushed the codex/nullable-external-id-map branch from 8cc525e to dda6130 Compare June 15, 2026 12:08

mergify Bot added ci-passed and removed ci-passed labels Jun 15, 2026

marcelo-cjl force-pushed the codex/nullable-external-id-map branch from dda6130 to 3745a01 Compare June 16, 2026 06:26

mergify Bot added ci-passed and removed ci-passed labels Jun 16, 2026

marcelo-cjl force-pushed the codex/nullable-external-id-map branch 2 times, most recently from 39fc115 to 8d65e1b Compare June 20, 2026 15:37

mergify Bot added the ci-passed label Jun 21, 2026

marcelo-cjl force-pushed the codex/nullable-external-id-map branch from 8d65e1b to c71da0e Compare June 22, 2026 13:24

mergify Bot removed the ci-passed label Jun 22, 2026

marcelo-cjl force-pushed the codex/nullable-external-id-map branch 3 times, most recently from c5895e4 to b141b03 Compare June 23, 2026 08:16

zilliztech deleted a comment from mergify Bot Jun 23, 2026

marcelo-cjl mentioned this pull request Jun 23, 2026

Support nullable external id mapping in the index layer #1687

Open

marcelo-cjl added the kind/enhancement label Jun 23, 2026

mergify Bot removed the do-not-merge/missing-related-issue label Jun 23, 2026

marcelo-cjl force-pushed the codex/nullable-external-id-map branch from b141b03 to 8ef741b Compare June 24, 2026 00:07

mergify Bot added the ci-passed label Jun 24, 2026

marcelo-cjl force-pushed the codex/nullable-external-id-map branch from 8ef741b to af54801 Compare June 24, 2026 15:35

mergify Bot removed the ci-passed label Jun 24, 2026

marcelo-cjl force-pushed the codex/nullable-external-id-map branch from af54801 to 01c245e Compare June 25, 2026 13:06

mergify Bot added the ci-passed label Jun 25, 2026

feat: support nullable external id mapping

1e5db0f

Signed-off-by: marcelo-cjl <marcelo.chen@zilliz.com>

marcelo-cjl force-pushed the codex/nullable-external-id-map branch from 01c245e to 1e5db0f Compare July 8, 2026 10:42

mergify Bot added ci-passed and removed ci-passed labels Jul 8, 2026

alexanderguzhva reviewed Jul 14, 2026

View reviewed changes

Uh oh!

Conversation

marcelo-cjl commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Uh oh!

mergify Bot commented Jun 10, 2026

Uh oh!

foxspy commented Jun 11, 2026

Uh oh!

foxspy commented Jun 11, 2026

Uh oh!

foxspy commented Jun 11, 2026

Uh oh!

foxspy commented Jun 11, 2026

Uh oh!

sre-ci-robot commented Jun 24, 2026

Uh oh!

marcelo-cjl commented Jun 25, 2026

Uh oh!

marcelo-cjl commented Jun 25, 2026

Uh oh!

marcelo-cjl commented Jun 25, 2026

Uh oh!

marcelo-cjl commented Jun 25, 2026

Uh oh!

alexanderguzhva commented Jun 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marcelo-cjl commented Jun 10, 2026 •

edited

Loading