Conversation
![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: 13e2a7efc1
ℹ️ 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".
Fix search functionality for CJK languages by addressing three issues:
1. Query sanitization: Switch from ASCII-only \w to Unicode-aware \p{L}\p{N}
2. Stemmer: Add character-level tokenization for CJK (e.g., "日本語" → "日 本 語")
3. Highlighter: Add CJK-aware highlighting without word boundaries
For SQLite FTS5: Store original content in main table, stemmed content in
FTS table. Use Search::Highlighter instead of FTS5's highlight() function
to display original text in search results.
13e2a7e to
fd83c65
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd83c659ba
ℹ️ 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".
… CJK boundary matching - Stem highlighter terms so searches like "running" highlight "run" in results - Add stem_query to preserve quoted phrases in SQLite MATCH queries - Fix word boundary regex failing at CJK/Latin transitions (Ruby \b issue) - Use case-insensitive matching for CJK snippet positioning
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa20528f9e
ℹ️ 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".
…hting - Unquoted CJK terms like 中文 now become "中 文" (phrase) in MATCH queries to preserve character order - CJK highlight regex is now case-insensitive for mixed CJK/Latin terms
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@jorgemanrubia This is a re-implementation of CJK search support, addressing the issues from the previous attempt (#2321). Would appreciate your review when you have a chance. Thanks! |
djmb
left a comment
djmb
left a comment
There was a problem hiding this comment.
A few thoughts:
This won't work with MySQL because by default it requires tokens to be 3 characters or more. We should check that it doesn't break anything though.
We are now going to double stem data, once with mittens in Ruby and then a second time with the porter stemmer in the FTS5 SQLite implementation. This maybe works fine, but it seems a bit iffy, for example if the two stemming algorithms do not work the same way.
Ideally we should change the table to tokenize='unicode61' to avoid the SQLite tokenization.
I think we also will need to do a full reindex in a migration so the correctly stemmed data is stored.
Finally we should add some tests with CJK data to searches_controller_test.rb for a more complete testing. The setup is quite slow for those tests because they are not transactional so we might want a single test that tests a bunch of different queries.
|
Thanks for the detailed review @djmb, and the feedback @jorgemanrubia! Updated in 973ff66:
|
There was a problem hiding this comment.
Pull request overview
This PR re-implements CJK (Chinese/Japanese/Korean) search support by separating stemmed storage/indexing from user-facing display, and replacing SQLite FTS5 highlight()/snippet() usage with an application-level Search::Highlighter to avoid displaying stemmed text.
Changes:
- Add CJK-aware stemming/tokenization and query stemming to support CJK matching in SQLite FTS.
- Update highlighting/snippet generation to be CJK-aware and to operate on original (display) text rather than FTS-returned stemmed text.
- Switch SQLite FTS tokenizer to
unicode61and add/adjust tests covering CJK search/highlighting/snippet behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
app/models/search.rb |
Introduces shared CJK_PATTERN constant for cross-component CJK detection. |
app/models/search/highlighter.rb |
Adds CJK-aware highlight/snippet logic and improves term handling (stemming-aware). |
app/models/search/query.rb |
Updates sanitization to preserve Unicode letters/numbers (enables CJK queries). |
app/models/search/stemmer.rb |
Implements character-level tokenization for CJK plus stem_query to produce FTS5-friendly queries. |
app/models/search/record/sqlite.rb |
Uses Search::Highlighter for display and stores stemmed content in the FTS table; stems queries for matching. |
app/models/search/record/trilogy.rb |
Refactors highlight helpers to highlight_full/highlight_snippet for consistency with SQLite path. |
db/migrate/20260304120000_change_search_fts_tokenizer_to_unicode61.rb |
Migrates SQLite FTS table to unicode61 and reindexes. |
db/schema_sqlite.rb |
Updates schema version and FTS virtual table definition to unicode61. |
db/cable_schema.rb |
Contains unrelated schema-dump changes not mentioned in the PR description. |
test/models/search/stemmer_test.rb |
Adds coverage for CJK stemming, mixed content, and stem_query behavior. |
test/models/search/highlighter_test.rb |
Adds CJK highlight/snippet tests and switches snippet tests to max_chars. |
test/models/card/searchable_test.rb |
Updates SQLite FTS expectation to reflect stemmed content storage. |
test/controllers/searches_controller_test.rb |
Adds integration coverage for CJK search results and highlight rendering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Switch FTS5 tokenizer from porter to unicode61 to eliminate double-stemming (Ruby Mittens + SQLite porter). Added migration with full reindex. - Unify snippet into a single character-based method, removing the separate CJK/western branches and cjk_dominant? check. - Split highlight(text, show:) into highlight_snippet/highlight_full in both SQLite and Trilogy modules. - Add CJK integration test to searches_controller_test (SQLite only, MySQL fulltext requires 3+ character tokens). - Revert unrelated cable_schema.rb changes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@Maklu Any updates ? I need this too. Thank you. |
Resolve conflict in SQLite matching scope: adopt the explicit INNER JOIN from main while keeping stemmed query from this branch.
|
@BANG88 Just rebased to resolve conflicts. Still waiting on maintainer review — hopefully it gets picked up soon. |
Bro. Thank you very much. |
* pr-2337-temp: Address review feedback: switch FTS5 to unicode61, unify snippet logic Wrap CJK tokens as phrases in FTS5 queries and fix mixed-term highlighting Address review feedback: fix stemmed highlighting, phrase search, and CJK boundary matching Add CJK (Chinese, Japanese, Korean) search support
|
Hello @Maklu , I am wondering why this PR has not been merged. I would like to submit PRs to the main branch in hopes of it ending up in the https://www.fizzy.do/ deployment but I am not sure if that actually ever happens with PRs submitted. Thoughts? |
|
@tushabe All the review feedback was addressed back in early March, so it's been waiting on a maintainer re-review for a couple of months now. Keeping the PR focused and responsive to review is about all we can control on our end. @djmb thanks again for the detailed review earlier. No rush at all, but whenever you have some time, I'd really appreciate another look — everything you flagged has been addressed since early March. |
|
Would this PR also add support for other characters, like åäö? I noticed the searching being broken for Swedish characters as well. |
# Conflicts: # db/schema.rb # db/schema_sqlite.rb
Query sanitization used to strip non-ASCII letters, breaking search
for accented characters (å, ä, ö, é, ...). The switch to \p{L}\p{N}
in Search::Query fixed this alongside CJK; cover it with a test that
runs on both SQLite and MySQL.
Claude-Session: https://claude.ai/code/session_01AHkZVHPX1kNFJU5fYu7o41
|
@AndreasNasman Yes, this fixes that too. On main, query sanitization strips any non-ASCII letter before the search runs (Ruby's One limitation: stemming is English-only, so inflected forms won't match each other (e.g. "hälsa" vs "hälsan"), but exact word matches work. |
highlight() inserted real <mark> HTML during per-term gsub passes, so a
later term matching the markup itself (e.g. "class", "text") corrupted
the output. Wrap matches in private-use sentinel characters instead and
swap in the real markup once all terms are processed.
Query sanitization and stemmer tokenization also treated combining marks
(\p{M}) as separators, splitting decomposed accents (NFD "café") and
scripts like Devanagari into fragments. Keep marks with their base
characters on both the indexing and query side.
Claude-Session: https://claude.ai/code/session_01AHkZVHPX1kNFJU5fYu7o41
|
Awesome @Maklu! 💪 |
Maklu
changed the title
|
Updated in a453c26 to work with the latest main and its ActiveSearch migration.
CJK substring search remains SQLite-only; MySQL ngram support is outside this PR’s scope. Ready for another review. |
Problem
Queries such as
中文,日本語,한국어, andhälsaare still stripped by the ASCII-only query sanitizer on main. SQLite's existing tokenizer also treats contiguous CJK text as one token, so a query such as中文cannot find中文测试卡片.Changes
This implementation uses the current ActiveSearch architecture:
portertokenizer for English; Ruby does not stem indexed SQLite content.Card to deleteand日本語, never stemmed or space-separated index content. Overlapping terms are merged before inserting ActiveSearch markers; ActiveSearch handles HTML escaping and per-field formatting.search:reindex.Scope
CJK character-level search is supported on SQLite. MySQL keeps whole Unicode words and its existing full-text token-size limits; this does not add CJK substring search or the MySQL ngram parser. MySQL ngram support remains a separate change.
Validation
a453c2665: full application and system tests on SQLite and MySQL, plus lint, security, Gemfile drift, and GitHub Actions audit.