fix: default BM25 tokenizer splits CJK characters for bare-term retrieval - #12837
Conversation
|
@Rainmemery is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
@Rainmemery thanks for opening the PR! Please address the failing checks and it would be great if you could figure out why the github diff is so off since I don't think your changes are 2K lines of changes |
43dc32b to
057961e
Compare
|
Thanks for the feedback @sjrl — I've rebuilt the branch and the diff should be clean now. Root cause: the PR branch was initially created from an outdated snapshot of my fork's main, which is why GitHub showed ~2K lines of unrelated changes. I've rebased the branch onto the latest upstream
Tests are re-running on the new head commit now. Let me know if anything else needs attention. |
|
@Rainmemery thanks for the updates! We do need you to sign the CLA agreement here #12837 (comment) for us to be able to accept your contribution. So please sign that when you can. |
90ad00c to
19616b6
Compare
|
Update: the Unit suite revealed that several o_dict() serialization tests asserted the old default regex (?u)\b\w+\b. Since the default changes with this PR, I updated those expectations to the new regex value. Locally all affected tests pass (575 passed). Waiting for CI to confirm. |
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
19616b6 to
faaf1e7
Compare
|
@sjrl CLA is now signed — the author email was the issue, I rewrote the commit to the noreply address linked to my account and the CLA bot now reports "All committers have signed the CLA". Thanks for flagging it. |
faaf1e7 to
f3364b2
Compare
|
@sjrl rebased the branch onto the latest |
sjrl
left a comment
There was a problem hiding this comment.
Three notes on the character ranges in the new default regex.
|
Thanks for the ping @sjrl, and thanks @Rainmemery for picking this up so quickly. I ran the PR's regex against the benchmark from the issue plus a few new ones. It fixes the reported bug, and I would like to flag one consequence before it becomes the default. All numbers below are What worksRecall goes from 0/10 to 10/10 on the issue's benchmark, with every relevant document at rank 1. Latin behaviour is unchanged ( The concern: single-syllable tokens cost precision in KoreanQuerying The unrelated document wins because it happens to contain 서 (from 서점, "bookstore") and 울 (from 울고, "crying"). Querying This is the part I can speak to as a native speaker: a single Hangul syllable is a very weak unit of meaning. 서 on its own can be 書 / 西 / 序 and more, and 울 can be the verb stem "cry" or part of 울타리 ("fence"); neither carries the identity of the word it came from. Splitting Korean into syllables is closer to splitting English into individual letters than into words. This is why Lucene's CJK analyzer emits bigrams rather than unigrams. Comparison
Bigrams recover the precision but lose single-syllable queries, which is exactly what Lucene's One practical obstacle if you do want bigrams: they cannot be expressed in the current API. Two smaller notes
Where I landThe status quo is unusable for Korean, so this is a clear improvement and I would rather have it than not. But it trades a total-recall failure for a precision failure, and which of those is the better default is a call for the maintainers rather than for me. If you want the precision back, I am happy to open a follow-up PR for a callable-tokenizer API plus a CJK bigram tokenizer, or to contribute the benchmark below as a test so the tradeoff stays visible. Benchmark scriptr"""Tokenizer comparison for haystack PR #12837 (haystack-ai==3.1.1).
Swaps only the BM25 tokenizer, holding corpus, retriever and BM25 algorithm fixed:
current the released default, r"(?u)\b\w+\b"
unigram this PR: one token per Hangul syllable / CJK ideograph / kana
bigram Lucene CJKBigramFilter style: overlapping 2-grams over CJK runs
both bigram + unigram, i.e. Lucene's outputUnigrams=true
"""
import re
from haystack import Document
from haystack.components.retrievers.in_memory import InMemoryBM25Retriever
from haystack.document_stores.in_memory import InMemoryDocumentStore
CJK = r"ᄀ-ᇿ-가--ヿ一-鿿"
CURRENT = r"(?u)\b\w+\b"
UNIGRAM = rf"[^\W{CJK}]+|[{CJK}]"
SPLIT = re.compile(rf"[^\W{CJK}]+|[{CJK}]+")
CJK_RUN = re.compile(rf"[{CJK}]+")
def cjk_tokenizer(*, bigrams: bool, unigrams: bool):
def tokenize(text: str) -> list[str]:
out: list[str] = []
for m in SPLIT.finditer(text):
s = m.group(0)
if not CJK_RUN.fullmatch(s):
out.append(s)
continue
if bigrams:
out.extend([s[i : i + 2] for i in range(len(s) - 1)] or [s])
if unigrams:
out.extend(list(s))
return out
return tokenize
def make_store(mode: str, docs: list[Document]) -> InMemoryDocumentStore:
regex = CURRENT if mode == "current" else UNIGRAM
store = InMemoryDocumentStore(bm25_tokenization_regex=regex)
if mode == "bigram":
store.tokenizer = cjk_tokenizer(bigrams=True, unigrams=False)
elif mode == "both":
store.tokenizer = cjk_tokenizer(bigrams=True, unigrams=True)
store.write_documents(docs)
return store
def search(store: InMemoryDocumentStore, query: str, top_k: int = 10) -> list[Document]:
return InMemoryBM25Retriever(store, top_k=top_k, scale_score=False).run(query=query)["documents"]
# One natural sentence per city, mentioning it once with an attached particle.
CITIES = [
("서울", "서울은 대한민국의 수도이며 인구가 가장 많다."),
("부산", "부산은 대한민국 제2의 도시이자 최대 항구이다."),
("제주", "제주에는 화산 활동으로 만들어진 독특한 지형이 많다."),
("대구", "대구를 방문하면 근대 골목길을 걸어볼 수 있다."),
("인천", "인천에서 출발하는 국제선 항공편이 가장 많다."),
("광주", "광주의 5월은 역사적으로 중요한 의미를 가진다."),
("대전", "대전은 과학 연구 단지가 밀집한 도시로 알려져 있다."),
("울산", "울산에는 자동차와 조선 산업 단지가 모여 있다."),
("세종", "세종으로 여러 중앙 행정 기관이 이전하였다."),
("수원", "수원에는 조선 시대에 쌓은 화성이 남아 있다."),
]
# Unrelated sentences that happen to contain 서 or 울 inside other words.
DISTRACTORS = [
"서점 앞에서 울고 있는 아이를 보았다.",
"서류를 모두 제출해야 접수가 완료된다.",
"울타리 너머로 개가 짖고 있었다.",
"서열을 정하는 회의가 길어졌다.",
"경기도는 서쪽 해안을 따라 갯벌이 넓다.",
]
SINGLE = ["산이 높고 물이 깊다.", "부산은 항구 도시이다.", "등산을 좋아한다."]
print(f"{'tokenizer':9} {'recall@10':>9} {'top-1':>7} {'서울 precision':>16} {'산 hits':>8} {'KO tokens':>10}")
for mode in ("current", "unigram", "bigram", "both"):
store = make_store(mode, [Document(content=s, meta={"city": c}) for c, s in CITIES])
recall = sum(1 for c, _ in CITIES if any(d.meta["city"] == c for d in search(store, c)))
top1 = sum(1 for c, _ in CITIES if (r := search(store, c)) and r[0].meta["city"] == c)
precision_docs = [Document(content=CITIES[0][1], meta={"relevant": True})]
precision_docs += [Document(content=s, meta={"relevant": False}) for s in DISTRACTORS]
ranked = search(make_store(mode, precision_docs), "서울", top_k=3)
precision = "no results" if not ranked else ("correct@1" if ranked[0].meta["relevant"] else "WRONG@1")
single = len(search(make_store(mode, [Document(content=s) for s in SINGLE]), "산", top_k=3))
tokens = len(store._tokenize_bm25(CITIES[0][1]))
print(f"{mode:9} {recall:>7}/10 {top1:>5}/10 {precision:>16} {single:>8} {tokens:>10}")
print("\nRanking for the query 서울:")
precision_docs = [Document(content=CITIES[0][1], meta={"relevant": True})]
precision_docs += [Document(content=s, meta={"relevant": False}) for s in DISTRACTORS]
for mode in ("unigram", "bigram"):
print(f" {mode}")
for i, d in enumerate(search(make_store(mode, precision_docs), "서울", top_k=3)):
print(f" {i + 1}. [{'right' if d.meta['relevant'] else 'wrong'}] {d.score:6.3f} {d.content}")
print("\nAverage document length, 20 Korean + 3 English documents:")
ko = [f"서울은 대한민국의 수도이며 인구가 가장 많다. 사례 {i}." for i in range(20)]
en = [
"Seoul is the capital of South Korea and has the largest population.",
"Busan is the second largest city in South Korea.",
"The capital city hosts the national government.",
]
for mode in ("current", "unigram"):
store = make_store(mode, [Document(content=c) for c in ko + en])
avgdl = sum(len(store._tokenize_bm25(c)) for c in ko + en) / len(ko + en)
scores = ", ".join(f"{d.score:.3f}" for d in search(store, "capital", top_k=3))
print(f" {mode:9} avgdl={avgdl:5.1f} scores for the English query 'capital': {scores}") |
|
An addendum, and a correction of my own: I wrote the comment above from the diff and the issue thread and missed that @sjrl had already left inline review threads on this file a day earlier. The NFD point I listed as my "smaller note 2" was his first — apologies for restating it as if it were new. His three technical notes all reproduce here, and two of them have a Korean angle worth adding. NFD (his third thread). Confirmed, and it is worse than "not improved": the two spellings share no terms at all. _tokenize_bm25("서울") # ['서', '울']
_tokenize_bm25(unicodedata.normalize("NFD", "서울")) # ['ᄉ', 'ᅥ', 'ᄋ', 'ᅮ', 'ᆯ']
# intersection: set()Normalizing next to CJK Compatibility Ideographs, U+F900–FAFF (his second thread). He noted this is the block Korean hanja uses; it is worth being precise about why, because it makes the gap asymmetric rather than merely incomplete. That block exists largely because of Korean: a hanja with several Korean readings gets one code point per reading, so 樂 is U+4E50 when read 락 and U+F914 when read 악. The same visible character therefore tokenizes two different ways: _tokenize_bm25("樂安民") # ['樂', '安', '民'] unified, U+4E00–9FFF
_tokenize_bm25("樂例類") # ['樂例類'] compatibility, U+F900–FAFFSo a Korean document that uses the reading-specific forms gets no benefit from this PR at all, while the same text in unified forms does. One block missing from his list: halfwidth Hangul jamo, U+FFA0–FFDC. _tokenize_bm25("ハン") # ['ハン'] halfwidth katakana, U+FF66–FF9F (already on the list)
_tokenize_bm25("ᄆᄇᄈ") # ['ᄆᄇᄈ'] halfwidth Hangul jamo, U+FFA0–FFDCSame class of gap, same fix. It shows up in the same exported/legacy data that halfwidth katakana comes from. On his first thread ( None of this changes where I landed above: the precision question is still the one I would want decided before this becomes the default. |
…eval Update the default bm25_tokenization_regex of InMemoryDocumentStore so CJK scripts are tokenized per character, enabling single-character bare-term retrieval for Chinese/Japanese/Korean. Also updates to_dict expectations affected by the new default. Refinements from review: - Extract the default pattern into _CJK_CHAR_CLASS / _DEFAULT_BM25_TOKENIZATION_REGEX so the source, the to_dict tests and the docs reference a single definition instead of a duplicated literal. - Narrow the kana ranges so punctuation such as the katakana middle dot (U+30FB), the kana iteration marks and the combining voicing marks no longer become standalone tokens (and no longer inflate doc_len), while keeping the prolonged sound mark (U+30FC). - Cover the remaining CJK/Jamo blocks: halfwidth katakana (U+FF66-U+FF9F), CJK Extension A (U+3400-U+4DBF), Katakana Phonetic Extensions (U+31F0-U+31FF), CJK Compatibility Ideographs (U+F900-U+FAFF) and Hangul Jamo Extended-A/B (U+A960-U+A97F, U+D7B0-U+D7FF). - Unicode NFC-normalize the text in _tokenize_bm25 so equivalent spellings (for example decomposed vs. composed Hangul) share the same terms. - Expand the bm25_tokenization_regex docstring and update the two hand-maintained docs pages (documentcleaner, documentsplitter) to the new serialized default.
f3364b2 to
6c7f7ab
Compare
|
Checked the new revision against the Korean cases. Two of the three things I raised are covered, and one block is still missing. NFC normalization — works. The two spellings now share all their tokens, where before they shared none: _tokenize_bm25("서울은") # ['서', '울', '은']
_tokenize_bm25(unicodedata.normalize("NFD", "서울은")) # ['서', '울', '은']CJK Compatibility Ideographs — works. Still missing: halfwidth Hangul jamo, U+FFA0–U+FFDC. The new class picks up halfwidth katakana at _tokenize_bm25("ハン") # ['ハ', 'ン'] halfwidth katakana, U+FF66–FF9F
_tokenize_bm25("ᄆᄇᄈ") # ['ᄆᄇᄈ'] halfwidth Hangul jamo, U+FFA0–FFDCIt is the Korean counterpart of the halfwidth katakana you added, from the same legacy and exported data, and it is one more range in r"ᅠ-ᅵ" # Halfwidth Hangul Jamo(U+FFA0 is the halfwidth filler and U+FFA1–FFDC the jamo; the halfwidth punctuation you deliberately excluded is below at U+FF61–FF65, so this range adds no punctuation.) Everything else in the revision reads well to me — pulling the pattern into The open question from my earlier comment is unchanged: per-character tokens still rank an unrelated document above the right one for |
|
@heeoneie thanks for all of your comments. In regards to
I agree that the best solution would be to move to a bigram tokenizer. And I also agree that adding such a change is should be a new issue and new PR and is probably out of scope for this PR. That being said would you say the changes in this PR still be worth adding even with its limitations? I also wanted to ask what your use case is for using the |
|
Is it worth adding as it stands? Yes. I would take this over the status quo without hesitation, for one reason above the others: the current failure is silent. A Korean query returns an empty list, with no error and no warning, and nothing in the result distinguishes "your tokenizer cannot see this word" from "no document matched". Noisy results are a problem a user can see and reason about; an empty list is one they attribute to their own data, their chunking, or their embedding model. In the benchmark on the issue, ten out of ten topics returned nothing at all, and the same corpus in English returned all ten. The precision cost is real, but it is bounded and visible, and it can be tracked in the follow-up issue rather than blocking this. On my use case — I should be straightforward: I am not running That said, I would gently push back on "it was originally intended for prototyping" as a reason to weigh this less. Prototyping is where a silent failure does the most damage, because the person hitting it has the least basis to suspect the library: they are usually new to Haystack, and if their first Korean retrieval returns nothing they are more likely to conclude that Haystack does not work for Korean than to go read the tokenization regex. On the follow-up. I would be glad to open the issue for the bigram tokenizer and write it up with the numbers I posted above: the precision comparison, the single-syllable-query tradeoff that makes pure bigrams insufficient on their own (Lucene solves it with |
|
@heeoneie thanks for the write up! Okay lets leave this PR as-is then and please open an issue where we can discuss what changes would be needed before opening a new PR. |
|
Opened #12867 with the numbers, the |
- Add halfwidth Hangul Jamo (U+FFA0-U+FFDC) to the CJK char class - Shorten the release note and drop the middle-dot claim (it was never a token on main) - Stop referencing private names in the public docstring, use single backticks - Assert full token lists in the CJK tokenization test - Add a retrieval test for the bare-term CJK query the PR fixes
heeoneie
left a comment
There was a problem hiding this comment.
All four are addressed, and I verified them against 7b9f785 rather than just reading the replies. Thanks for the quick turnaround.
Halfwidth Hangul range. U+FFA0–U+FFDC is exactly the block: U+FFA0 is HALFWIDTH HANGUL FILLER and U+FFDC is HALFWIDTH HANGUL LETTER I, U+FF9F below it is the katakana semi-voiced mark already covered, and U+FFDD above it is unassigned. There are nine unassigned code points inside the range, which is harmless in a character class. Including the filler is also consistent with U+3164 HANGUL FILLER, which the Hangul Compatibility Jamo range already picks up.
Dropping the middle-dot sentence was right. I checked both directions: U+30FB is category Po, so \w never matched it on main, and the new katakana range \u30a1-\u30fa\u30fc deliberately steps around it, so it is not emitted before or after this PR. The sentence was claiming a fix that was never a bug.
Test assertions. I ran the new regex over the four new/changed cases and they all hold, including \uffb1\uffb2\uffb3 → three tokens. _tokenize_bm25 does unicodedata.normalize("NFC", text.lower()), so the shortened docstring's "lowercased and NFC-normalized" is accurate.
Korean reads correctly. The two sentences in the new retrieval test are natural, and 서울은 / 부산은 are the right shape for the bug being pinned.
Two non-blocking notes:
-
test_bm25_retrieval_with_cjk_bare_term_queryusestop_k=1. The 부산 document shares no token with the query서울, so it scores 0 and is dropped regardless;len(results) == 1is therefore satisfied by construction. Withtop_k=2the same test would also assert that the non-matching document stays out, which is the other half of what the fix does. Entirely optional. -
Worth knowing rather than fixing: NFC does not fold halfwidth or compatibility jamo into syllables (that is NFKC), so halfwidth-jamo text still only matches halfwidth-jamo queries. That is the same behavior halfwidth katakana already has here, so the PR is self-consistent; I mention it only so nobody later reads the halfwidth addition as making the forms interchangeable.
LGTM from the native-speaker side.
Fixes #12836.
Problem
InMemoryDocumentStore's defaultbm25_tokenization_regex(r"(?u)\b\w+\b") treats every Hangul syllable as a word character, so a Korean noun never splits from an attached particle —서울은tokenizes as the single token서울은instead of["서울", "은"]. A bare-noun query like서울then has zero BM25 term overlap with documents where the noun only appears with a particle attached, which is nearly always in Korean prose. The same applies to CJK ideographs and kana.Fix
Change the default regex so that:
\u4e00-\u9fff, kana\u3040-\u30ff) becomes its own token — the standard char-level tokenization IR systems use for CJK text.r"[^\W\u1100-\u11ff\u3130-\u318f\uac00-\ud7af\u3040-\u30ff\u4e00-\u9fff]+|[\u1100-\u11ff\u3130-\u318f\uac00-\ud7af\u3040-\u30ff\u4e00-\u9fff]"Verification
_tokenize_bm25("서울은 대한민국의 수도")now yields 서/울/은/… (no noun+particle glued token).bm25_retrieval(query="서울")returns the document whose content only contains서울은."Luna is a dog"→["luna", "is", "a", "dog"],"Seoul 2026"→["seoul", "2026"]. Existing single-char token tests still pass (34 BM25 tests green in the in-memory store suite).test_bm25_tokenization_splits_cjk_characters.