Skip to content

Reject ID selectors when IndexShards shifts shard IDs - #5582

Open
rioyu123 wants to merge 1 commit into
facebookresearch:mainfrom
rioyu123:codex/fix-sharded-selector-id-space
Open

Reject ID selectors when IndexShards shifts shard IDs#5582
rioyu123 wants to merge 1 commit into
facebookresearch:mainfrom
rioyu123:codex/fix-sharded-selector-id-space

Conversation

@rioyu123

@rioyu123 rioyu123 commented Sep 5, 2026

Copy link
Copy Markdown

Related to #5581.

Summary

This PR proposes a narrow fail-fast boundary for selector searches when
IndexShards applies nonzero offsets to shard IDs. It prevents the incorrect
filtered results in #5581 without introducing a search-parameter cloning API
or mutating caller-owned parameters across worker threads.

The check runs before any shard search. Searches without a selector,
successive_ids=false, and configurations where all shard offsets are zero
retain their existing behavior. A nonzero offset also triggers the check when
that shard is currently empty; the boundary is based on the configured ID
offsets. The public search documentation explains
the limitation and the need for globally assigned shard IDs when using
successive_ids=false to preserve a global ID space.

Design question

This is a defensive error, not full support for translating global-ID
selectors into each shard's local ID space. This implementation is ready for review; feedback on the preferred approach
in #5581 is welcome. If per-shard selector
adaptation is preferred, that needs a separate mechanism that preserves
derived search parameters without sharing mutable state between workers.

This change covers the IndexShardsTemplate::search implementation used by
IndexShards and IndexBinaryShards. It does not change the separate
IndexShardsIVF::search override.

Validation

  • Built the candidate Faiss CPU library from
    2ed4c106e9fb9686e7727e5daf8ad6ad1e164109 with GCC 13.3 / C++20, generic
    CPU code and BLAS/LAPACK.
  • The final test_threaded_index.cpp against the unmodified library:
    2 failed, 6 passed. Both new rejection tests fail because no exception
    is raised.
  • Candidate threaded-index tests: 8 passed. Coverage includes serial and
    threaded searches, both floating-point failure cases, binary shards,
    unchanged output buffers and parameters, no sub-search before rejection,
    and the no-selector / no-translation controls.
  • Combined test_threaded_index.cpp, test_binary_flat.cpp and
    test_params_override.cpp: 28 passed.
  • clang-format 21.1.8 --dry-run --Werror on the three changed files and
    git diff --check passed.

Tests were compiled directly with the repository-pinned GoogleTest against
the source-built library. Full repository CI, Python bindings, GPU builds
and other platforms were not run locally.

@meta-cla

meta-cla Bot commented Sep 5, 2026

Copy link
Copy Markdown

Hi @rioyu123!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@meta-cla meta-cla Bot added the CLA Signed label Sep 5, 2026
@meta-cla

meta-cla Bot commented Sep 5, 2026

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@rioyu123
rioyu123 marked this pull request as ready for review September 5, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant