Repository navigation
fix(retrieval): unicode-aware tokenization; empty chunk text is never a relevant hit - #45
Conversation
…n-ASCII letter: BM25 over CJK text returned nothing, 'München' split into ['m','nchen']. Unicode-aware pattern + casefold; CJK runs emit each character. Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Updated the tokenization process to be casefolded and unicode-aware, allowing for better handling of multi-script tokens and CJK characters.
…by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Added a check to handle empty text cases in relevance evaluation.
…er Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Removed unused regex import and custom tokenize function.
…sions Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Add tests for tokenization and relevance checks with Unicode and accented characters.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e14382165d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _CJK_CHAR.search(word): | ||
| if len(word) > 1: | ||
| tokens.append(word) | ||
| tokens.extend(word) |
There was a problem hiding this comment.
When Latin text directly adjoins Han or kana, as in Python東京 or 使用Python开发, TOKEN_PATTERN treats the entire string as one word; finding any CJK character then causes this branch to emit the combined run and every individual character, but never python. Consequently, a Python query no longer matches such a BM25 document even though it did with the previous tokenizer, and the shared TF-IDF and lexical-overlap consumers are affected too. Split mixed-script runs or emit intact non-CJK subruns while expanding only the CJK portions.
Useful? React with 👍 / 👎.
| still match, plus the whole run so exact phrases score highest. | ||
| """ | ||
| tokens: List[str] = [] | ||
| for match in TOKEN_PATTERN.finditer(text.casefold()): |
There was a problem hiding this comment.
When indexed text uses decomposed Unicode from PDF/web extraction but the query uses its precomposed equivalent, casefold() alone does not normalize them. For example, Mu\u0308nchen becomes ['mu', 'nchen'] because the combining diaeresis breaks the regex match, while München becomes ['münchen'], yielding no BM25 match; the old tokenizer at least shared nchen, so this is a recall regression for canonically equivalent input. Normalize both forms to a common Unicode normalization form before applying the token regex.
Useful? React with 👍 / 👎.
| if len(word) > 1: | ||
| tokens.append(word) | ||
| tokens.extend(word) |
There was a problem hiding this comment.
For a multi-character CJK query within a normal unspaced sentence, the whole-run token cannot provide the promised phrase boost: 東京は日本の首都です produces one full-sentence token plus character unigrams, whereas query 東京 produces 東京, 東, and 京, so only the two independent characters match. A document containing 東 and 京 far apart therefore receives the same matching terms and can outrank the actual phrase based on length or term frequency. Emit overlapping CJK n-grams or otherwise retain substring boundaries rather than relying only on the entire run and unigrams.
Useful? React with 👍 / 👎.
…ned-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Add NFC normalization and improve tokenization logic.
|
Deployment failed for project intellirag-live-own-track with the following error: Learn More: https://vercel.com/own-track?upgradeToPro=build-rate-limit |
|
Deployment failed for project irag-fix-0904 with the following error: Learn More: https://vercel.com/own-track?upgradeToPro=build-rate-limit |
|
Deployment failed for project intellirag with the following error: Learn More: https://vercel.com/own-track?upgradeToPro=build-rate-limit |
…essions Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
|
@codex review All three findings addressed, each with failing-first regressions:
Retrieval + evaluation + pipeline: 62 passed vs 51 on main (+11 new across both rounds); 3 failures in those packages are identical on pristine main. Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2743373996
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| is_cjk = bool(_CJK_CHAR.match(word[i])) | ||
| j = i + 1 | ||
| while j < n and bool(_CJK_CHAR.match(word[j])) == is_cjk: | ||
| j += 1 |
There was a problem hiding this comment.
When Latin text directly adjoins another non-CJK script, both sides are classified as is_cjk=False and emitted as one token. This occurs naturally with Korean particles: tokenize("Python으로") produces ['python으로'], so a Python query produces no BM25 hit, whereas the previous ASCII tokenizer indexed python. Split non-CJK runs at script transitions as well, rather than only separating Han/kana from everything else.
Useful? React with 👍 / 👎.
| TOKEN_PATTERN = re.compile(r"[a-z0-9]+") | ||
| # Unicode-aware: letters and digits from ANY script form word tokens, so | ||
| # "München" stays one token instead of splitting into ["m", "nchen"]. | ||
| TOKEN_PATTERN = re.compile(r"[^\W_]+") |
There was a problem hiding this comment.
For scripts where vowel signs and viramas remain combining marks after NFC, [^^\W_]-style word matching drops those marks and collapses distinct words to the same token sequence. For example, की, का, कि, and कु all tokenize as ['क'], so a BM25 query for one can rank documents containing any of the others identically; the same problem affects Bengali, Telugu, Tamil, and other supported Unicode scripts. Keep combining marks attached to their preceding base character when forming non-CJK word tokens.
Useful? React with 👍 / 👎.
…plits every script boundary Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Refactor tokenization logic to handle mixed scripts more effectively, ensuring proper segmentation and script identification.
…it-in-word regressions Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
|
@codex review Both new findings addressed:
New failing-first regressions cover Latin-Hangul mixing, Devanagari combining marks, and digits inside words (RED on the previous tokenizer, GREEN now). Suite: 65 passed with only the 3 pre-existing eval/factory failures unchanged from main. Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29db5f276d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if script is None: | ||
| # Digits and unattributed characters inherit the current script. | ||
| segment += ch | ||
| continue |
There was a problem hiding this comment.
When digits adjoin CJK text, inheriting the CJK script causes each digit to be emitted only as a unigram or bigram. For example, 東京2026年 produces 20, 02, and 26 but not 2026, so BM25Index(...).search("2026") returns no result; the previous tokenizer did index that numeric run. Preserve contiguous numeric tokens in addition to any CJK n-grams so dates, versions, and identifiers remain searchable.
Useful? React with 👍 / 👎.
| # Han ideographs and Japanese kana are not space-separated, so a run of | ||
| # them cannot rely on word boundaries; they tokenize per character. | ||
| _CJK_SCRIPTS = frozenset({"CJK", "HIRAGANA", "KATAKANA"}) |
There was a problem hiding this comment.
For Thai and other scripts that commonly omit word-separating spaces, limiting subword emission to this CJK set leaves an entire sentence as one token. For example, the document กรุงเทพมหานครเป็นเมืองหลวงของประเทศไทย receives one token while the query ประเทศไทย receives another, so BM25 returns no hit despite the literal word being present. Include suitable word breaking or character n-grams for these scripts rather than treating every non-CJK run as a single word.
Useful? React with 👍 / 👎.
…aceless scripts Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Updated handling of scripts that do not use spaces between words to include Thai, Lao, Khmer, and Myanmar. Adjusted tokenization logic to accommodate digit runs while maintaining script association.
…age Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
|
@codex review Both findings addressed:
Two more failing-first regressions (RED on the previous head, GREEN now). Suite: 67 passed with only the 3 pre-existing eval/factory failures unchanged from main. Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a244d5c8e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # etc. - NFC leaves many of them decomposed, and dropping them collapses | ||
| # distinct words like "की" / "का" / "कि" onto one token). | ||
| def _is_word_char(ch: str) -> bool: | ||
| return ch.isalnum() or unicodedata.category(ch).startswith("M") |
There was a problem hiding this comment.
When a combining mark follows a non-word symbol, treating it as a standalone word character creates meaningless shared tokens. In particular, the variation selector in common emoji forms means tokenize("❤️"), tokenize("☀️"), and tokenize("✈️") all produce the same token, so a BM25 search for a heart can return unrelated sun or airplane chunks with equal scores. Keep marks only when they extend an existing alphanumeric token, rather than starting tokens on their own.
Useful? React with 👍 / 👎.
…Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
…nside words Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
|
@codex review Fixed: tokens now start only on letters/digits - a detached combining mark (like the emoji variation selector) no longer forms its own token, so Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 104bf7f238
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tokens.extend(seg) | ||
| tokens.extend(seg[k : k + 2] for k in range(len(seg) - 1)) |
There was a problem hiding this comment.
For Thai, Lao, Khmer, and Myanmar words, tokens.extend(seg) iterates Unicode code points, so combining marks accepted into a word become standalone query terms. For example, tokenize("กิ") emits "ิ", allowing a BM25 index containing only "มิ" to return a positive hit for "กิ" solely because both contain that vowel mark; the shared lexical-overlap consumers inherit the same false match. Build unigrams and bigrams from grapheme clusters (base plus attached marks) rather than individual code points.
Useful? React with 👍 / 👎.
| name = unicodedata.name(ch, "") | ||
| return name.split(" ")[0] if name else None |
There was a problem hiding this comment.
When Japanese text contains common marks such as 々 or ー, taking the first word of the Unicode name classifies them as IDEOGRAPHIC or KATAKANA-HIRAGANA rather than CJK/Katakana. _emit_word therefore splits around them and loses adjacency evidence—for example, tokenize("人々") is only ["人", "々"], and in a BM25 index "人と時々会う" can rank above "人々が集まる" for that query even though only the latter contains the phrase. Treat these characters as inheriting the surrounding Japanese script, or use Unicode Script Extensions, so their phrase n-grams are preserved.
Useful? React with 👍 / 👎.
…ion marks inherit script Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
…eep adjacency Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
|
@codex review Both findings addressed:
Both RED before the fix, GREEN after; retrieval/eval/pipeline 71 passed with only main's 3 pre-existing failures. Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d85cb72d98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if clusters and _is_mark(ch): | ||
| clusters[-1] += ch |
There was a problem hiding this comment.
Because _clusters attaches only Unicode category-M characters, the newly inherited 々 and ー remain standalone clusters and are emitted as searchable unigrams. Thus tokenize("ユーザー") and tokenize("スーパー") share only ー, yet a BM25 index containing only スーパー returns it for the ユーザー query; similarly, 時々 matches 人々 solely through 々. Preserve these marks in adjacency n-grams without emitting them as independent terms.
Useful? React with 👍 / 👎.
| # Digits inherit the surrounding script but stay searchable as | ||
| # whole runs: "東京2026年" must still match a "2026" query. | ||
| tokens.extend(m.group(0) for m in _DIGIT_RUN.finditer(seg) if len(m.group(0)) > 2) |
There was a problem hiding this comment.
Digit-run extraction occurs only for _NGRAM_SCRIPTS, so ASCII numbers adjoining other Unicode scripts are absorbed into an opaque whole-word token. For example, tokenize("إصدار2026") produces only إصدار2026, and BM25Index([("d", "إصدار2026")]).search("2026") returns nothing; the previous ASCII tokenizer indexed 2026, so this regresses searches for versions and identifiers in Arabic, Devanagari, and other non-ngram scripts. Emit contiguous digit runs independently for all script segments.
Useful? React with 👍 / 👎.
| if script != segment_script: | ||
| segments.append((segment, segment_script)) | ||
| segment = ch | ||
| segment_script = script |
There was a problem hiding this comment.
When a Japanese word transitions between kanji and kana, this split prevents any n-gram from spanning the boundary. Consequently, tokenize("食べる") is identical to tokenize("食、べる"), so a BM25 index containing the punctuated decoy before the actual phrase gives both the same score and ranks the decoy first. Emit an adjacency n-gram across contiguous Japanese-script segments so normal mixed-script words retain phrase evidence.
Useful? React with 👍 / 👎.
| precomposed query must still match ("Mu\u0308nchen" == "München"). | ||
| """ | ||
| tokens: List[str] = [] | ||
| normalized = unicodedata.normalize("NFC", text.casefold()) |
There was a problem hiding this comment.
NFC preserves compatibility characters, so visually equivalent text from East Asian documents remains in different tokens from an ordinary query. For example, tokenize("Ｐｙｔｈｏｎ") yields ｐｙｔｈｏｎ, and a BM25 index containing that full-width spelling returns nothing for Python; half-width katakana similarly fails to match its standard-width equivalent. Apply compatibility normalization, such as NFKC followed by case folding, so these common presentation forms remain searchable.
Useful? React with 👍 / 👎.
…ide any script, kanji/kana bigrams, NFKC Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Refactor tokenization logic for Japanese scripts and update normalization method.
…na adjacency, full-width forms Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
|
Fixed all 4:
4 new regression tests (test_iteration_marks_are_not_standalone_unigrams, test_digit_runs_beside_non_latin_scripts, test_kanji_kana_boundary_keeps_bigram, test_compatibility_forms_match_plain_query), RED before, GREEN after; full libs/workers suite 312 passed, 26 pre-existing failures (Docker Postgres); unused re import removed. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c27ad6159
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Digits and unattributed characters inherit the current script - | ||
| # except digits beside a non-Latin script, which form their own | ||
| # whole token so "إصدار2026" keeps a searchable "2026". | ||
| if ch.isdigit() and segment_script not in (None, "LATIN", "DIGIT"): |
There was a problem hiding this comment.
When digits precede a non-Latin word, segment_script is still None, so this condition leaves them in the pending segment and the following script absorbs them. For example, tokenize("2026إصدار") returns only "2026إصدار", and a BM25 query for 2026 cannot find that document; 2026東京 similarly emits digit unigrams and bigrams but not the complete 2026 token. Handle an initial digit run independently when its following character starts a non-Latin script.
Useful? React with 👍 / 👎.
| # NFKC folds compatibility forms (full-width Latin, half-width katakana) | ||
| # onto their ordinary equivalents so extracted East Asian text matches a | ||
| # plain query; casefold first per Unicode default caseless matching. | ||
| normalized = unicodedata.normalize("NFKC", text.casefold()) |
There was a problem hiding this comment.
Case-folding before NFKC misses compatibility characters whose decomposition introduces uppercase letters. For example, tokenize("㎒") produces "MHz" while tokenize("MHz") produces "mhz", and mathematical 𝐀 similarly fails to match a. Normalize before case-folding (and normalize again afterward if needed) so all compatibility-derived letters receive the same casing.
Useful? React with 👍 / 👎.
| # script buckets ("IDEOGRAPHIC ITERATION MARK", "KATAKANA-HIRAGANA | ||
| # PROLONGED SOUND MARK"); they inherit the surrounding script so "人々" and | ||
| # "ユーザー" stay single segments. | ||
| _INHERIT_CODEPOINTS = frozenset({"\u3005", "\u3006", "\u3007", "\u30fc"}) |
There was a problem hiding this comment.
U+3007 is IDEOGRAPHIC NUMBER ZERO, not an iteration or prolonged mark, but including it here causes the n-gram path to suppress it as a unigram. Consequently, a standalone 〇 query produces "〇", while a document such as 二〇二六 omits that unigram and cannot match the query. Classify this character as CJK rather than as a mark that should be filtered.
Useful? React with 👍 / 👎.
| # script buckets ("IDEOGRAPHIC ITERATION MARK", "KATAKANA-HIRAGANA | ||
| # PROLONGED SOUND MARK"); they inherit the surrounding script so "人々" and | ||
| # "ユーザー" stay single segments. | ||
| _INHERIT_CODEPOINTS = frozenset({"\u3005", "\u3006", "\u3007", "\u30fc"}) |
There was a problem hiding this comment.
The remaining hiragana and katakana iteration marks (ゝ, ゞ, ヽ, and ヾ) are naturally classified into n-gram scripts but are absent from this exclusion set, so they are still emitted as shared unigrams. For example, an index containing only さゝ returns it for the unrelated query くゝ solely because both contain ゝ; include these marks in the adjacency-only handling used for 々 and ー.
Useful? React with 👍 / 👎.
… zero, kana iteration marks Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Updated handling of Japanese iteration and prolonged marks, clarified comments on digit handling beside non-Latin scripts, and improved normalization process for Unicode compatibility.
…ero, kana iteration marks Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Added tests for tokenization behavior with leading digits, compatibility forms, ideographic zero, and kana iteration marks.
|
Fixed all 4:
4 new regression tests (leading digit runs, squared-unit casefold, ideographic zero, kana iteration marks), RED before, GREEN after; full libs/workers suite 316 passed, 26 pre-existing failures. @codex reviewFixed all 4:
4 new regression tests (leading digit runs, squared-unit casefold, ideographic zero, kana iteration marks), RED before, GREEN after; full libs/workers suite 316 passed, 26 pre-existing failures. @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Finding 1: silent recall loss for non-ASCII text
keyword.pytokenized withre.compile(r"[a-z0-9]+"), dropping every non-ASCII letter.Before: a BM25 index with one document
東京は日本の首都ですreturned[]for query東京;Münchentokenized to["m", "nchen"]. Any CJK or accented content was invisible to keyword and hybrid retrieval.After:
TOKEN_PATTERNis unicode-aware ([^\W_]+, letters/digits from any script) overcasefold()ed text (Straße->strasse). CJK runs additionally emit each character (Han/kana text is not space-separated, so a single-ideograph query must still match) plus the whole run so exact phrases score highest:The TF-IDF mock embedder in
pipeline/embedders.pyhad its own ASCII-only copy of the same pattern and now sharestokenizefromkeyword.py.Finding 2: empty chunk text counted as a relevant hit
_is_relevantinretrieval_benchmark.pysubstring-matched both ways (ref in text or text in ref). A chunk with missing text (dense.pymaps absent text to"") made"" in ref_lowertrue against every reference, inflating recall/MRR/NDCG.Before:
_is_relevant("missing-text-chunk", "", sample)->TrueAfter: empty text returns
False; an explicitrelevant_chunk_idsmatch still counts.Tests
Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com