Repository navigation
fix: keep combining marks inside words in BM25 tokenization - #755
nicoloangileri wants to merge 2 commits into
Conversation
|
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; 7 remain after this review. 📝 WalkthroughWalkthroughThe 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 The change is ready to merge after normal checks; no actionable issue was identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to 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 Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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 |
…m25-combining-marks # Conflicts: # tests/test_sparse_embeddings.py
|
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! |
All Submissions:
What this fixes
SimpleTokenizer.tokenizesplits on[^\w], andremove_non_alphanumericon[^\w\s]. Python's\wdoes 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, soBm25(language="tamil")andBm25(language="arabic")are affected directly:['தம', 'ழ', 'ஒர', 'பழம', 'ய', 'ன', 'ம', 'ழ', ...]['தமிழ்', 'ஒரு', 'பழமை', 'மொழி', 'தமிழ்நாடு', 'தியா', 'ஒரு', 'மாநிலம்']['الط', 'ال', 'ء', 'الم']['ذهب', 'طالب', 'الي', 'مدرس']The same happens with the default English model on Hindi text, where
हिन्दी भाषाbecomes['ह', 'न', 'द', 'भ', 'ष'].Change
get_all_marks()incommon/utils.pyreturns the category M code points as character class ranges. It is computed once, following the existingget_all_punctuation().remove_non_alphanumericandSimpleTokenizer.tokenizeadd these marks to the word class.get_all_punctuation()has atBm25init.remove_non_alphanumericis also used by miniCOIL for unknown words, so it now keeps those words whole too.Checks
test_combining_marks_do_not_split_words(tokenizer andremove_non_alphanumericon Tamil, Hindi and Arabic) andtest_stem_words_with_combining_marks(Arabic and Tamil through the stemmer). All three fail on main.tests/test_sparse_embeddings.pypass, and so doruff0.3.4 and the CImypycommand.qdrant/bm25inference (v1.19.1,language: arabic), vocalized Arabic now gives the same sparse vectors as the server, which it did not before.Notes
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 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.