fix: prevent BM25 query token ID overflow - #763
RAMZI0TO99 wants to merge 1 commit into
Conversation
|
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
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthrough
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The BM25 overflow fix appears ready to merge after normal checks; no actionable risk remains identified. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to 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 Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
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:
Before on Windows:
After, matching the expected result:
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:
and nine relevant existing cases.
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
eight all-state GitHub searches and complete inspection of all 32 open PR
file diffs.