Clarify Chinese query segmentation scope - #1014
Conversation
safishamsi
left a comment
safishamsi
left a comment
There was a problem hiding this comment.
Good contribution — CJK segmentation is a real gap and the jieba + bigram fallback design is sound. Two small things before merge: (1) _has_cjk() only covers U+4E00–U+9FFF (Chinese), so Japanese (Hiragana/Katakana) and Korean (Hangul) users get no segmentation — either broaden the ranges or rename to _has_chinese so the scope is honest; (2) jieba is imported on every _segment_cjk call — cache it at module level with a try/except guard. Could you fix those and open a fresh PR against v8?
|
Good contribution — CJK segmentation is a real gap and the jieba + bigram fallback design is sound. Two small things before merge: (1) |
|
Okay, I'll revise it and resubmit the PR later. |
fc13be6 to
951eebe
Compare
candy-Tong
changed the title
d8282a7 to
794b087
Compare
Keep Graphify query segmentation focused on Chinese terms: rename the CJK helpers and extra to Chinese scope, cache the optional jieba import at module load, and keep the original query term when jieba is unavailable. Constraint: Reviewer asked either to broaden Hiragana/Katakana/Hangul support or rename CJK helpers; user chose Chinese-only because Japanese segmentation accuracy is uncertain. Constraint: User requested no bigram fallback; without jieba, Chinese query terms are kept whole rather than segmented. Rejected: Broaden to Japanese and Korean segmentation | jieba is Chinese-oriented and the user explicitly limited scope to Chinese. Rejected: Character bigram fallback | user prefers no segmentation when jieba is unavailable. Rejected: Mock jieba segmentation in tests | use real jieba behavior for optional integration coverage and skip no-jieba assertions when jieba is installed. Confidence: high Scope-risk: narrow Directive: Do not label this path as CJK unless Hiragana/Katakana/Hangul segmentation is intentionally supported and tested. Tested: uv run --with pytest pytest tests/test_serve.py tests/test_query_cli.py tests/test_benchmark.py (66 passed, 2 skipped) Tested: uv run --with pytest --with jieba pytest tests/test_serve.py -k "chinese or non_chinese" (5 passed, 1 skipped) Tested: graphify update . Not-tested: Full test suite. Co-authored-by: OmX <omx@oh-my-codex.dev>
794b087 to
fc0224c
Compare
|
The issues raised here (rename |
2 participants
Summary
v8branch (68863a7).jiebaimport at module level.jiebais available; whenjiebais unavailable, keep the original query term unchanged.jiebabehavior in optional segmentation tests instead of mocking the tokenizer.Fixes #962
Closes #1013
Tests
uv run --with pytest pytest tests/test_serve.py tests/test_query_cli.py tests/test_benchmark.py— 66 passed, 2 skippeduv run --with pytest --with jieba pytest tests/test_serve.py -k "chinese or non_chinese"— 5 passed, 1 skippedgraphify update .