Skip to content

fix: prevent BM25 query token ID overflow - #763

Closed
RAMZI0TO99 wants to merge 1 commit into
qdrant:mainfrom
RAMZI0TO99:fix/bm25-query-overflow
Closed

RAMZI0TO99 wants to merge 1 commit into
qdrant:mainfrom
RAMZI0TO99:fix/bm25-query-overflow

Conversation

@RAMZI0TO99

Copy link
Copy Markdown
Contributor

BM25 query embeddings store abs(mmh3.hash(token)) in an int32 array. For
the valid token ad1u66pi, the signed hash is -2147483648 and its absolute
value is 2147483648, exceeding the int32 maximum by one. Document embedding
accepts this ID, but query embedding raises OverflowError with NumPy 2.

Store query indices as int64, which SparseEmbedding already accepts. Keep
compute_token_id unchanged so queries continue to match existing document
IDs. Add 11 regression cases using the actual mmh3 dependency and BM25
implementation, with local model paths and no downloads.

Reproduction:

from tempfile import TemporaryDirectory
import mmh3
from fastembed.sparse.bm25 import Bm25

token = "ad1u66pi"
with TemporaryDirectory() as directory:
    model = Bm25(
        "Qdrant/bm25",
        disable_stemmer=True,
        specific_model_path=directory,
        cache_dir=directory,
        local_files_only=True,
    )
    print("Signed hash:", mmh3.hash(token))
    print("Token ID:", model.compute_token_id(token))
    print("Document indices:", next(model.embed([token])).indices.tolist())
    print("Query indices:", next(model.query_embed(token)).indices.tolist())

Before on Windows:

Signed hash: -2147483648
Token ID: 2147483648
Document indices: [2147483648]
OverflowError: Python int too large to convert to C long

After, matching the expected result:

Signed hash: -2147483648
Token ID: 2147483648
Document indices: [2147483648]
Query indices: [2147483648]

Environment: Windows 11 build 10.0.26200, Python 3.13.5, NumPy 2.3.5,
mmh3 5.3.1, FastEmbed 0.8.1 at upstream main commit
7d36728. This reproduction imports the
actual package and hash dependency, without copied methods or substituted
hash functions. The boundary failure is rare; this report does not claim
frequent production impact.

Validation:

  • Before the fix: 7 new cases failed, 4 passed.
  • After: 20 passed on Python 3.13.5 / NumPy 2.3.5, covering all 11 new cases
    and nine relevant existing cases.
  • All 11 new cases also pass on Python 3.10.11 / NumPy 2.2.6.
  • Ruff 0.3.4 lint/format and staged whitespace checks pass.
  • The full model-download suite and complete CI matrix were not run locally.

Coverage includes the real boundary hash, document/query ID agreement,
uppercase and punctuation normalization, repeated tokens, list and generator
inputs, ordinary tokens, and empty queries. Query unit values created by
np.ones_like also widen from int32 to int64; their numeric value remains one.

Related: #369 discusses abs-hash collisions, and #290's proposed hashing
change was rejected because it would change existing IDs. This patch retains
the established hashing behavior and fixes the array conversion boundary.
#290 (comment)

All Submissions

  • Followed the guidelines in the Contributing document.
  • Checked for other open PRs for the same change: no duplicate found in
    eight all-state GitHub searches and complete inspection of all 32 open PR
    file diffs.

@RAMZI0TO99
RAMZI0TO99 requested a review from joein as a code owner October 3, 2026 14:09
@coderabbitai

coderabbitai Bot commented Oct 3, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 24a9b748-ace9-4a59-9486-7875f6de53ca
📥 Commits

Reviewing files that changed from the base of the PR and between 7d36728 and 5371e71.

📒 Files selected for processing (2)
  • fastembed/sparse/bm25.py
  • tests/test_bm25_query_index_overflow.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

Bm25.query_embed now creates query token indices as np.int64; query values remain ones. New tests cover boundary token IDs, normalization, list and generator inputs, deduplication, and empty queries.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 5371e

The BM25 overflow fix appears ready to merge after normal checks; no actionable risk remains identified.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 5371e

The wider query indices preserve existing token identities and are already supported by the shared embedding format. No material security risk introduced or worsened by this change was identified within the inspected library boundary.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated effect is confined to query-result representation at the library boundary. The shared consumer supports the wider indices; exposure across deployed services, tenants, or external integrations is not established by the available evidence.

Trust Boundaries and Controls

  • inferred — Caller-controlled query strings follow the same processing path before and after this change. Widening the output array does not introduce a new trust transition, privileged operation, or bypass of an existing control in the inspected method.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preventing overflow in BM25 query token IDs.
Description check ✅ Passed The description explains the overflow issue, the int64 fix, and the regression tests. It is directly related to the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant