Skip to content

fix: keep combining marks inside words in BM25 tokenization - #755

Open
nicoloangileri wants to merge 2 commits into
qdrant:mainfrom
nicoloangileri:fix/bm25-combining-marks
Open

nicoloangileri wants to merge 2 commits into
qdrant:mainfrom
nicoloangileri:fix/bm25-combining-marks

Conversation

@nicoloangileri

Copy link
Copy Markdown

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

What this fixes

SimpleTokenizer.tokenize splits on [^\w], and remove_non_alphanumeric on [^\w\s]. Python's \w does not match combining marks (Unicode category M). In Tamil and Devanagari the vowel signs are marks, and in Arabic the harakat are. BM25 therefore cuts every word in these scripts at each mark and keeps only the pieces in between.

Tamil and Arabic are in supported_languages, with Snowball stemmers, so Bm25(language="tamil") and Bm25(language="arabic") are affected directly:

from fastembed.common.utils import remove_non_alphanumeric
from fastembed.sparse.bm25 import Bm25

for language, text in [
    ("tamil", "தமிழ் ஒரு பழமையான மொழி. தமிழ்நாடு இந்தியாவின் ஒரு மாநிலம்."),
    ("arabic", "ذَهَبَ الطَّالِبُ إِلَى المَدْرَسَةِ"),
]:
    model = Bm25("Qdrant/bm25", language=language)
    print(model._stem(model.tokenizer.tokenize(remove_non_alphanumeric(text))))
main this PR
Tamil, 8 words 21 fragments: ['தம', 'ழ', 'ஒர', 'பழம', 'ய', 'ன', 'ம', 'ழ', ...] ['தமிழ்', 'ஒரு', 'பழமை', 'மொழி', 'தமிழ்நாடு', 'தியா', 'ஒரு', 'மாநிலம்']
Arabic, 4 words ['الط', 'ال', 'ء', 'الم'] ['ذهب', 'طالب', 'الي', 'مدرس']

The same happens with the default English model on Hindi text, where हिन्दी भाषा becomes ['ह', 'न', 'द', 'भ', 'ष'].

Change

  • get_all_marks() in common/utils.py returns the category M code points as character class ranges. It is computed once, following the existing get_all_punctuation().
  • remove_non_alphanumeric and SimpleTokenizer.tokenize add these marks to the word class.
  • ASCII input contains no marks, so it keeps the original patterns. For such text the output is the same by construction, and the speed does not change: 30k lines of English Markdown took 0.299 s on main and 0.301 s here. Without this fast path, the longer class made that corpus 2x slower.
  • The first non-ASCII call pays a one-time scan of the Unicode table, about 0.1-0.25 s here. That is the same kind of cost get_all_punctuation() has at Bm25 init.

remove_non_alphanumeric is also used by miniCOIL for unknown words, so it now keeps those words whole too.

Checks

  • New tests: test_combining_marks_do_not_split_words (tokenizer and remove_non_alphanumeric on Tamil, Hindi and Arabic) and test_stem_words_with_combining_marks (Arabic and Tamil through the stemmer). All three fail on main.
  • The BM25 tests in tests/test_sparse_embeddings.py pass, and so do ruff 0.3.4 and the CI mypy command.
  • On 3000 lines of README and docs text, document and query embeddings are identical to main for all 6000 outputs.
  • Compared with Qdrant's server-side qdrant/bm25 inference (v1.19.1, language: arabic), vocalized Arabic now gives the same sparse vectors as the server, which it did not before.

Notes

  • Vectors already stored for text that contains marks will change. Those collections need to be re-embedded to benefit from the fix.
  • Tamil and Hindi still differ from the server: the server splits on characters that are not char::is_alphanumeric, and the virama/pulli signs are not alphabetic, so it breaks words at them. Keeping those words whole seemed the right behaviour for the tokenizer here.
  • While comparing with the server I also noticed that it splits on _ while fastembed keeps it (wait_until). I left that out of this PR because changing either side changes existing vectors. I can open an issue for it if that is useful.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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: 92b00acd-0f6d-4b9b-be53-3e67c81afd2c
📥 Commits

Reviewing files that changed from the base of the PR and between 01074e8 and d94cf80.

📒 Files selected for processing (2)
  • fastembed/common/utils.py
  • tests/test_sparse_embeddings.py

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


📝 Walkthrough

Walkthrough

The change adds cached regex patterns that preserve Unicode combining marks when filtering text and tokenizing non-ASCII input. ASCII input uses plain patterns. New tests cover Tamil, Devanagari, and Arabic text, including Arabic and Tamil BM25 stemming.

Priority: ➖ Normal

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to d94cf

The change is ready to merge after normal checks; no actionable issue was identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 01074

The change remains within text preprocessing and does not introduce a new privileged access path in the reviewed code. However, text containing combining marks can produce different sparse vectors, so existing indexes may be incompatible with queries generated after upgrading.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The established affected path is supplied text flowing through BM25 preprocessing into token counts or sparse vectors. No new production caller or privileged sink was identified in the reviewed relationships; external tenant, service, and data-store exposure is not established by the supplied evidence.

Trust Boundaries and Controls

  • observed — The reviewed filtering and tokenization functions perform text normalization, not authorization or identity validation. Existing BM25 caller relationships remain unchanged, so mark preservation does not bypass an access-control check in these paths.

Resilience and Maintainability Implications

  • observed — The added caches wrap zero-argument helpers and contain Unicode-derived ranges or compiled patterns. Their cache keys do not contain attacker-supplied text, avoiding input-driven cache cardinality growth.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 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 summarizes the main change: keeping combining marks inside words during BM25 tokenization.
Description check ✅ Passed The description explains the tokenization problem, the changes, affected languages, tests, performance, and compatibility impact.
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

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.

…m25-combining-marks

# Conflicts:
#	tests/test_sparse_embeddings.py
@nicoloangileri

Copy link
Copy Markdown
Author

Hi @joein, a gentle ping on this one. The workflow runs are waiting for maintainer approval (first-time contributor), and the merge conflict with main is resolved. Happy to adjust anything. Thanks!

This branch has not been deployed

No deployments
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