Skip to content

feat(search): support CJK queries in ticket and article search - #3712

Open
katsushi2441 wants to merge 2 commits into
frappe:developfrom
katsushi2441:cjk-search
Open

feat(search): support CJK queries in ticket and article search#3712
katsushi2441 wants to merge 2 commits into
frappe:developfrom
katsushi2441:cjk-search

Conversation

@katsushi2441

Copy link
Copy Markdown

Problem

Search drops every Japanese, Chinese and Korean character before it reaches the backend.

Search.clean_query() (helpdesk/search.py) and sanitize_query() (helpdesk/api/article.py) both strip anything outside [a-zA-Z0-9\s]:

unsafe_chars = re.compile(r"[^a-zA-Z0-9\s]")

So a CJK-only query such as パスワード becomes an empty string and the search returns nothing. Sites running in ja / zh / ko cannot search their own tickets or knowledge base at all. The repo ships ko.po and zh.po, so these users exist today.

A second, smaller issue: when the first pass finds too few results, api/article.py falls back to TextBlob noun phrases. Its corpus is English-only, so for a CJK query the fallback replaces valid results with nothing.

What this does

  • helpdesk/search_i18n.py (new) — NFKC normalisation that keeps letters and digits in any script, CJK detection, and n-gram helpers. Pure functions, no Frappe imports.
  • Index a cjk_terms field (2- and 3-grams) for both the Redis and the SQLite backends, so substring matching works without pulling in a CJK tokenizer (FTS5's default tokenizer does not split CJK text).
  • Expand CJK runs in the query into 3-grams at search time. Latin terms pass through unchanged, so existing behaviour for English queries is preserved.
  • Skip the TextBlob fallback when the query contains CJK.
  • Bump the SQLite index filename (helpdesk_search_v2.db) so existing sites rebuild with the new column.
  • Unit tests for normalisation, detection, n-grams and query expansion.

Verification

helpdesk/test_search_i18n.py passes (5 tests). One test builds a real FTS5 table and asserts that パスワード matches a document titled パスワード変更の手順 and does not match an unrelated document.

Example of the query rewriting:

input normalised expanded
パスワード変更! パスワード変更 パスワ スワー ワード ード変 ド変更
VPN接続エラー vpn接続エラー vpn 接続エ 続エラ エラー

Notes

  • Existing English/Latin queries take the same code path as before; only the CJK branch is new.
  • The cjk_terms field is stripped from SQLite results before they are returned.
  • I am happy to adjust the approach (for example a custom FTS5 tokenizer instead of n-grams) if you prefer a different direction.

Search silently dropped every Japanese, Chinese and Korean character.
`Search.clean_query()` and `sanitize_query()` stripped anything outside
`[a-zA-Z0-9\s]`, so a CJK-only query became an empty string and returned
nothing. Sites running in ja/zh/ko could not search their own tickets.

- add `helpdesk/search_i18n.py`: NFKC normalisation that keeps letters and
  digits in any script, CJK detection, and n-gram helpers
- index a `cjk_terms` field (2- and 3-grams) for both the Redis and the
  SQLite backends so substring matches work without a CJK tokenizer
- expand CJK runs in the query into 3-grams; Latin terms are untouched
- skip the TextBlob noun-phrase fallback for CJK queries (its corpus is
  English-only and it discarded valid results)
- bump the SQLite index filename so existing sites rebuild with the new column
- add unit tests for normalisation, detection, n-grams and query expansion

Verified with FTS5: "パスワード" matches a document titled
"パスワード変更の手順" while an unrelated document does not match.
@mergify

mergify Bot commented Aug 21, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Reviews (2): Last reviewed commit: "fix(search): rebuild the Redis index whe..." | Re-trigger Greptile

The SQLite backend gets a new index filename, so upgraded sites rebuild it.
The Redis index keeps its name, and `index_exists()` only compared the
document count — which stays correct across schema changes. An index built
before `cjk_terms` existed therefore looked valid, was never rebuilt, and
could not serve the CJK query path.

`index_exists()` now also compares the declared schema against the fields
reported by FT.INFO and treats a missing field as "index absent". When the
attribute shape cannot be parsed it falls back to the previous behaviour
rather than forcing a rebuild.

Adds tests for the FT.INFO attribute shapes (flat lists, byte strings and
mappings) and for the stale-index case.
@katsushi2441

Copy link
Copy Markdown
Author

Thanks — the Redis migration gap was a real one, and I have pushed a fix in 7a72cfc.

The problem: Search.index_exists() only compared num_docs against the record count. That number stays correct across schema changes, so an index built before cjk_terms existed looked valid, was never rebuilt by build_index_if_not_exists(), and could not serve the new CJK query path. The SQLite backend was already covered because its index filename changed; Redis keeps the same index name, so nothing forced a rebuild there.

The fix: index_exists() now also compares the declared schema against the fields reported by FT.INFO and treats a missing field as "index absent", so the existing rebuild path takes over.

existing = indexed_field_names(ftinfo.get("attributes"))
if existing and not self.index_fields() <= existing:
    return self._index_exists  # False -> rebuild

Two deliberate choices:

  • This is generic rather than a one-off cjk_terms check, so any future field addition triggers the same rebuild instead of silently serving a stale index.
  • If the attribute shape cannot be parsed, indexed_field_names() returns an empty set and the check is skipped. FT.INFO reports attributes as flat lists on some versions and as mappings on others, and I would rather fall back to the previous behaviour than force a rebuild on an unrecognised shape.

indexed_field_names() lives in search_i18n.py so the tests stay runnable with plain python -m pytest, like the rest of this PR. Tests now cover flat lists, byte strings, mappings, the legacy no-identifier shape, the stale-index case and the unknown shape. 10 tests pass.

Happy to adjust if you would rather bump the Redis index name instead — that is simpler, but it loses the ability to notice future schema drift.

katsushi2441 added a commit to katsushi2441/frappe-helpdesk-jp that referenced this pull request Aug 23, 2026
jp ブランチの日本語検索には、インデックスの作り直し判定が無かった。
Redis側は「文書数が合っていれば既存インデックスを使う」という判定
だったので、スキーマに項目が増えた後も古いインデックスを使い続け、
新しい項目に対する検索が黙って空振りする。

上流PR frappe#3712 (cjk-search) で入れた indexed_field_names() による
スキーマ照合を jp にも取り込む。search_sqlite の INDEX_NAME も
上流と同じ helpdesk_search_v2.db に揃えた(jp独自の
helpdesk_search_ja_v1.db は、同じ目的の暫定対処だったので不要)。

競合した4ファイルは cjk-search 側を採用。日本語資産
(ja.po 7,218行 / docs/INSTALL_JA.md / validate_japanese_locale.py)
はそのまま残っていることを確認済み。

検証: validate_japanese_locale.py → 1,499/1,499 translated, OK

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
katsushi2441 added a commit to katsushi2441/frappe-helpdesk-jp that referenced this pull request Aug 23, 2026
krayin-jp では冒頭に「本家への還元」としてPR番号を出しているが、
こちらには無く、代わりに「Crowdinを通じて提案します」と書いていた。
実際には直接プルリクエストを2本出しているので、実態と食い違っていた。

  PR frappe#3713 日本語ロケール(ja.po・1,499メッセージ)
  PR frappe#3712 日本語(CJK)検索対応

読んでから導入を判断できるよう、解説記事と相談先も載せた。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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