MB-69881: Re-architect vector search #62

CascadingRadium · 2025-12-26T13:29:18Z

Remove the obsolete faiss_SearchParametersIVF_new_with_sel API and consolidate eligible-vector handling through bitmap-based selectors.
Replace faiss_get_lists_for_keys with faiss_count_ivf_list_vectors, which efficiently iterates the eligible vector bitmap (IDSelectorBitmap) to count per-IVF-list vector membership, avoiding key materialization and enabling direct centroid eligibility computation in the storage layer.

Copilot

Pull request overview

This PR re-architects vector search by replacing key-based list retrieval with bitmap-based vector counting for IVF indexes. It removes the obsolete faiss_SearchParametersIVF_new_with_sel and faiss_get_lists_for_keys APIs, replacing them with count_ivf_list_vectors which efficiently counts vectors per IVF list using bitmap iteration instead of key materialization.

Key Changes:

New count_ivf_list_vectors method that iterates through IDSelectorBitmap to count vectors per IVF list
Removal of get_lists_for_keys and faiss_SearchParametersIVF_new_with_sel APIs
Type signature change for faiss_Search_closest_eligible_centroids from FaissIndex* to const FaissIndexIVF*

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File	Description
faiss/IndexIVF.h	Adds declaration and documentation for new `count_ivf_list_vectors` method; removes `get_lists_for_keys` declaration
faiss/IndexIVF.cpp	Implements `count_ivf_list_vectors` with bitmap iteration logic; removes `get_lists_for_keys` implementation
c_api/IndexIVF_c_ex.h	Updates C API: removes `faiss_SearchParametersIVF_new_with_sel` and `faiss_get_lists_for_keys`; adds `faiss_count_ivf_list_vectors`; updates `faiss_Search_closest_eligible_centroids` signature
c_api/IndexIVF_c_ex.cpp	Implements C API wrapper for `count_ivf_list_vectors`; removes implementations of deleted functions; updates `faiss_Search_closest_eligible_centroids` to use const cast

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-28T06:39:36Z

faiss/IndexIVF.cpp

+        FAISS_THROW_MSG("count_ivf_list_vectors supports only IDSelectorBitmap");
+    }
+    const uint8_t* bitmap = bitmap_sel->bitmap;
+    const size_t nbytes = bitmap_sel->n;


There is a semantic mismatch between how the bitmap size is interpreted. According to IDSelectorBitmap's documentation and constructor (line 107 in IDSelector.h), the parameter 'n' represents "size of the bitmap array" (in bytes). However, on line 953, 'nbytes' is assigned from 'bitmap_sel->n', treating it as the number of bytes to iterate.

Looking at the is_member implementation in IDSelector.cpp line 120, it checks "if ((i >> 3) >= n)" which confirms that 'n' is indeed the number of bytes in the bitmap array. This means the implementation is correct.

However, there's potential confusion: if a caller constructs an IDSelectorBitmap with a bitmap sized for ntotal IDs (requiring ceil(ntotal/8) bytes), but accidentally passes a different value for n, this function would either under-iterate (missing valid IDs) or over-iterate (checking bits beyond the intended range). The safety check at line 966 mitigates over-iteration but doesn't detect under-iteration.

Consider adding an assertion or check that verifies: nbytes >= (ntotal + 7) / 8 to ensure the bitmap is large enough to cover all vectors in the index.

Suggested change

const size_t nbytes = bitmap_sel->n;

const size_t nbytes = bitmap_sel->n;

const size_t min_bytes = (ntotal + 7) / 8;

FAISS_ASSERT_MSG(

nbytes >= min_bytes,

"IDSelectorBitmap too small for index ntotal; bitmap size (bytes) "

"must be >= ceil(ntotal/8)");

Copilot · 2025-12-28T06:39:37Z

faiss/IndexIVF.h

+     * @param list_counts      - Output array of size list_counts_size (must be == nlist).
+     *                          On return, list_counts[i] contains the number
+     *                          of selected vectors assigned to IVF list i.
+     *                          Must be initialized to zero by the caller.


The documentation comment for the 'list_counts' parameter states "Must be initialized to zero by the caller." However, the implementation uses the '++' operator (line 973 in IndexIVF.cpp) which increments the existing value. This means the assertion should verify that the array is zero-initialized, or the documentation should be updated to clarify that this is a requirement. Without verifying this precondition, if the caller passes an uninitialized or non-zero array, the results will be incorrect.

Suggested change

* Must be initialized to zero by the caller.

* Precondition: every element of list_counts must

* be initialized to 0 by the caller before

* calling this function; otherwise the resulting

* per-list counts will be incorrect.

Copilot · 2025-12-28T06:39:37Z

c_api/IndexIVF_c_ex.h

+    @param list_counts       - Output array of size index->nlist.
+                               On return, list_counts[i] contains the number
+                               of selected vectors assigned to IVF list i.


The documentation states "Must be initialized to zero by the caller" but there is no corresponding note in the C API documentation. The C API documentation at lines 40-42 should also mention this requirement: "Output array of size index->nlist. Must be initialized to zero by the caller. On return, list_counts[i] contains the number of selected vectors assigned to IVF list i."

Suggested change

@param list_counts - Output array of size index->nlist.

On return, list_counts[i] contains the number

of selected vectors assigned to IVF list i.

@param list_counts - Output array of size index->nlist. Must be

initialized to zero by the caller. On return,

list_counts[i] contains the number of selected

vectors assigned to IVF list i.

Copilot · 2025-12-28T06:39:37Z

c_api/IndexIVF_c_ex.h

 */
 int faiss_Search_closest_eligible_centroids(
-        FaissIndex* index,
+        const FaissIndexIVF* index,


The function signature changed the index parameter from FaissIndex* to const FaissIndexIVF*. This is a breaking API change that will cause compilation errors for existing code that passes a FaissIndex*. While the change makes sense (the function specifically needs an IVF index and doesn't modify it), this should be clearly documented as a breaking change in the PR description or release notes to warn API users.

Suggested change

const FaissIndexIVF* index,

const FaissIndex* index,

Copilot · 2025-12-28T06:39:38Z

faiss/IndexIVF.cpp

+void IndexIVF::count_ivf_list_vectors(
+        idx_t* list_counts,
+        size_t list_counts_size,
+        const faiss::SearchParameters* params) const {
+    FAISS_ASSERT(list_counts != nullptr && list_counts_size > 0 && list_counts_size == nlist);
+    FAISS_ASSERT(params != nullptr && params->sel != nullptr);
+    FAISS_ASSERT(direct_map.type != DirectMap::NoMap);
+    const IDSelector* sel = params->sel;
+    // Optimized for bitmap selectors only
+    const IDSelectorBitmap* bitmap_sel = dynamic_cast<const IDSelectorBitmap*>(sel);
+    if (!bitmap_sel) {
+        FAISS_THROW_MSG("count_ivf_list_vectors supports only IDSelectorBitmap");
+    }
+    const uint8_t* bitmap = bitmap_sel->bitmap;
+    const size_t nbytes = bitmap_sel->n;
+    // Iterate over bitmap bytes
+    for (size_t byte_idx = 0; byte_idx < nbytes; ++byte_idx) {
+        uint8_t byte = bitmap[byte_idx];
+        if (byte == 0) {
+            continue; // fast skip
+        }
+        // Iterate over bits in the byte
+        for (uint8_t bit = 0; bit < 8; ++bit) {
+            if ((byte & (1 << bit)) == 0) {
+                continue;
+            }
+            idx_t id = (byte_idx << 3) + bit;
+            if (id >= ntotal) {
+                continue; // Safety check: skip invalid ids
+            }
+            uint64_t list_no = lo_listno(direct_map.get(id));
+            if (list_no >= nlist) {
+                continue; // Safety check: skip invalid list numbers
+            }
+            list_counts[list_no]++;
+        }
    }
 }


The new function count_ivf_list_vectors lacks test coverage. Given that the repository has comprehensive testing (e.g., test_lowlevel_ivf.cpp, test_ivf_index.cpp), this new functionality should be tested to ensure it correctly counts vectors per IVF list when using IDSelectorBitmap, handles edge cases (empty bitmaps, invalid IDs, etc.), and validates the preconditions (direct_map set, proper bitmap size, etc.).

Copilot · 2025-12-28T06:39:38Z

faiss/IndexIVF.cpp

+        idx_t* list_counts,
+        size_t list_counts_size,
+        const faiss::SearchParameters* params) const {
+    FAISS_ASSERT(list_counts != nullptr && list_counts_size > 0 && list_counts_size == nlist);


The assertion combines multiple conditions with logical AND, making it harder to debug when it fails. If any of these conditions fails, the user won't know which specific condition caused the assertion. Consider splitting this into separate assertions with descriptive messages:

FAISS_ASSERT(list_counts != nullptr); FAISS_ASSERT(list_counts_size > 0); FAISS_ASSERT(list_counts_size == nlist);

Or at minimum, use FAISS_THROW_IF_NOT_MSG for better error messages.

Suggested change

FAISS_ASSERT(list_counts != nullptr && list_counts_size > 0 && list_counts_size == nlist);

FAISS_ASSERT(list_counts != nullptr);

FAISS_ASSERT(list_counts_size > 0);

FAISS_ASSERT(list_counts_size == nlist);

CascadingRadium added 3 commits December 24, 2025 23:14

new api

680521b

re order methods

effcd2d

cleanup

2cbdbb4

CascadingRadium changed the title ~~wip~~ MB-69881: Re-architect vector search Dec 28, 2025

CascadingRadium added this to Vector Search v2 Dec 28, 2025

github-project-automation bot moved this to Todo in Vector Search v2 Dec 28, 2025

CascadingRadium moved this from Todo to Done in Vector Search v2 Dec 28, 2025

CascadingRadium requested review from Likith101, Nischal1729, Thejas-bhat, abhinavdangeti, capemox, Copilot, deepkaran and maneuvertomars December 28, 2025 06:35

Copilot started reviewing on behalf of CascadingRadium December 28, 2025 06:36 View session

Copilot AI reviewed Dec 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

MB-69881: Re-architect vector search #62

MB-69881: Re-architect vector search #62

Uh oh!

CascadingRadium commented Dec 26, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 28, 2025

Uh oh!

Copilot AI Dec 28, 2025

Uh oh!

Copilot AI Dec 28, 2025

Uh oh!

Copilot AI Dec 28, 2025

Uh oh!

Copilot AI Dec 28, 2025

Uh oh!

Copilot AI Dec 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-    const size_t nbytes = bitmap_sel->n;
+    const size_t nbytes = bitmap_sel->n;
+    const size_t min_bytes = (ntotal + 7) / 8;
+    FAISS_ASSERT_MSG(
+            nbytes >= min_bytes,
+            "IDSelectorBitmap too small for index ntotal; bitmap size (bytes) "
+            "must be >= ceil(ntotal/8)");

-     *                          Must be initialized to zero by the caller.
+     *                          Precondition: every element of list_counts must
+     *                          be initialized to 0 by the caller before
+     *                          calling this function; otherwise the resulting
+     *                          per-list counts will be incorrect.

-    FAISS_ASSERT(list_counts != nullptr && list_counts_size > 0 && list_counts_size == nlist);
+    FAISS_ASSERT(list_counts != nullptr);
+    FAISS_ASSERT(list_counts_size > 0);
+    FAISS_ASSERT(list_counts_size == nlist);

MB-69881: Re-architect vector search #62

Are you sure you want to change the base?

MB-69881: Re-architect vector search #62

Uh oh!

Conversation

CascadingRadium commented Dec 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CascadingRadium commented Dec 26, 2025 •

edited

Loading