[feature](inverted-index) Add Japanese (Kuromoji) morphological analyzer#64667
[feature](inverted-index) Add Japanese (Kuromoji) morphological analyzer#64667nishant94 wants to merge 16 commits into
Conversation
nishant94
requested review from
BiteTheDDDDt,
airborne12 and
zclllyybb
as code owners
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
@nishant94 have you tried icu analyzer? because I think icu could handle many different languages. |
@yiguolei The ICU Analyzer is not good as the Kuromoji. There is huge difference between icu and kuromoji when it comes to morphology of the Japanese words. So I think it worth it adding this new parser. |
|
Is the code under |
This is original code but it is modeled on Apache Lucene's kuromoji. |
FE UT Coverage ReportIncrement line coverage |
nishant94
force-pushed
the
feat/kuromoji-japanese-analyzer
branch
from
389fcfb to
b79db3c
Compare
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
nishant94
force-pushed
the
feat/kuromoji-japanese-analyzer
branch
from
db0ee69 to
06b4ef6
Compare
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
- Modified error messages to include 'kuromoji' parser in the parser mode validation. - Enhanced tests for the Japanese analyzer to assert expected tokenization results.
- Introduced a new configuration option `enable_kuromoji_analyzer` to toggle the Kuromoji analyzer functionality. - Updated unit tests to validate the behavior of the Kuromoji analyzer when enabled and disabled. - Modified tests to enable the Kuromoji analyzer for specific test cases.
- Updated the namespace for Kuromoji components from `doris::segment_v2::kuromoji` to `doris::segment_v2::inverted_index::kuromoji` across multiple files for better organization and clarity.
- Updated the CMake configuration to ensure the required Kuromoji dictionary files are present at build time, failing the build if any are missing. - Modified the KuromojiAnalyzer and KuromojiTokenizer to throw exceptions when the dictionary is not loaded, preventing silent fallbacks to per-codepoint tokenization. - Improved error handling and validation in the dictionary loading process to ensure robust operation. - Updated unit tests to validate the new behavior, ensuring that missing dictionaries trigger appropriate errors.
nishant94
force-pushed
the
feat/kuromoji-japanese-analyzer
branch
from
698dfb5 to
cb16636
Compare
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
[Non-blocking] Viterbi hot path allocates per byte position; +16% measured with a small changehere is what I measured (Release build, single pipeline task, sql cache off, 50×1MB natural text,
To be fair, the other rows are not apples-to-apples baselines: icu does much lighter work on Japanese than a full lattice/Viterbi morphological analysis, and ik/chinese run on a Chinese corpus, so some gap is expected and inherent to what kuromoji does. I'm only including them as a rough sense of scale — kuromoji is the slowest builtin analyzer but stays within the same order of magnitude, so I don't see this as blocking. That said, a profile shows a good chunk of the time goes to avoidable heap allocations, so there are two cheap wins in
Prototype of exactly these two changes: 19.3s → 16.2s, byte-identical tokenizer output on the 50MB corpus. The --- a/be/src/storage/index/inverted/analyzer/kuromoji/kuromoji_viterbi.cpp
+++ b/be/src/storage/index/inverted/analyzer/kuromoji/kuromoji_viterbi.cpp
@@ -121,18 +121,25 @@ void KuromojiViterbi::segment(std::string_view text, std::vector<KuromojiMorphem
}
std::vector<VNode> nodes;
- std::vector<std::vector<int>> ending_at(n + 1); // node indices ending at each byte position
+ // Intrusive per-end-position chain: end_head[e] is the most recent node index
+ // ending at byte position e, end_next[i] links to the previous one. This avoids
+ // allocating n+1 std::vector objects per document.
+ std::vector<int32_t> end_head(n + 1, -1);
+ std::vector<int32_t> end_next;
// BOS (index 0): ends at position 0, context id 0, zero cost.
nodes.push_back(VNode {0, 0, 0, 0, 0, false, 0, 0, -1});
- ending_at[0].push_back(0);
+ end_next.push_back(-1);
+ end_head[0] = 0;
// Add a node and relax it against all nodes ending at its start position.
auto add_node = [&](uint32_t s, uint32_t e, int16_t lid, int16_t rid, int16_t wcost, bool known,
uint32_t wid) {
int64_t best = KMJ_INF;
int best_prev = -1;
- for (int pe : ending_at[s]) {
+ // Chain is iterated newest-first; "<=" keeps the oldest node on cost ties,
+ // matching the original insertion-order "<" selection exactly.
+ for (int pe = end_head[s]; pe >= 0; pe = end_next[pe]) {
const VNode& pv = nodes[static_cast<std::size_t>(pe)];
if (pv.total_cost >= KMJ_INF) {
continue;
@@ -140,7 +147,7 @@ void KuromojiViterbi::segment(std::string_view text, std::vector<KuromojiMorphem
const int64_t c =
pv.total_cost + _dict.connection_cost(static_cast<uint32_t>(pv.right_id),
static_cast<uint32_t>(lid));
- if (c < best) {
+ if (c <= best) {
best = c;
best_prev = pe;
}
@@ -154,12 +161,15 @@ void KuromojiViterbi::segment(std::string_view text, std::vector<KuromojiMorphem
const auto idx = static_cast<int>(nodes.size());
nodes.push_back(
VNode {s, e, lid, rid, wcost, known, wid, best + wcost + penalty, best_prev});
- ending_at[e].push_back(idx);
+ end_next.push_back(end_head[e]);
+ end_head[e] = idx;
};
uint32_t pos = 0;
+ // Reused across positions; common_prefix_search clears it on entry.
+ std::vector<KuromojiDictionary::PrefixMatch> matches;
while (pos < n) {
- if (ending_at[pos].empty()) {
+ if (end_head[pos] < 0) {
pos += decode_utf8(text, pos).len; // unreachable boundary; skip
continue;
}
@@ -167,7 +177,6 @@ void KuromojiViterbi::segment(std::string_view text, std::vector<KuromojiMorphem
const auto before = nodes.size();
// System-dictionary words (common-prefix search).
- std::vector<KuromojiDictionary::PrefixMatch> matches;
_dict.common_prefix_search(text.data() + pos, n - pos, &matches);
bool any_known = false;
for (const auto& mt : matches) {
@@ -219,14 +228,14 @@ void KuromojiViterbi::segment(std::string_view text, std::vector<KuromojiMorphem
// EOS: best node ending at n connected to the EOS context (id 0).
int64_t best = KMJ_INF;
int best_prev = -1;
- for (int pe : ending_at[n]) {
+ for (int pe = end_head[n]; pe >= 0; pe = end_next[pe]) {
const VNode& pv = nodes[static_cast<std::size_t>(pe)];
if (pv.total_cost >= KMJ_INF) {
continue;
}
const int64_t c =
pv.total_cost + _dict.connection_cost(static_cast<uint32_t>(pv.right_id), 0);
- if (c < best) {
+ if (c <= best) {
best = c;
best_prev = pe;
} |
- Replaced the `ending_at` vector with `end_head` and `end_next` for better memory management and performance during node processing. - Updated node addition and traversal logic to utilize the new data structures, enhancing the segmenter's efficiency in handling word segmentation.
@Ryan19929 I am glad to see you spent time for benchmarking analyzers. Love to see these benchmarks. Also your optimization suggestion is pretty helpful. I took the refernce from it and swapped the ending_at vector-of-vectors for the intrusive Thank you Ryan !! |
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
- Changed the values in the `unk.per_category[CAT_DEFAULT]` entry from `{5, 5, 4769, "unk-default"}` to `{2, 2, 4769, "unk-default"}` to correct the test setup.
- Modified CMake configuration to conditionally include the Kuromoji dictionary files only for non-test builds (MAKE_TEST=ON). - Adjusted the custom target for generating the Kuromoji dictionary to reflect the new conditional behavior, ensuring it remains a manual target during unit-test builds.
|
run buildall |
|
/review |
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review result: request changes.
I reviewed the full PR diff and existing review threads, with subagent convergence after the final merged comment set. I did not run builds or tests per the review prompt.
Critical checkpoints:
- Goal/coverage: the PR adds Kuromoji analyzer support, dictionary generation/packaging, FE validation, and tests, but several correctness and build/test issues remain.
- Data correctness / FE-BE contract: analyzer-only Kuromoji configuration is accepted by FE but fails later because parser-mode defaults ignore the analyzer key.
- Runtime correctness / lifecycle: malformed dictionary artifacts can still install mmap-backed pointers that later crash or read invalid metadata.
- Search semantics: search/extended mode currently drops the original compound token instead of preserving the default Lucene/OpenSearch-compatible behavior.
- Build/config: the new dictionary target depends on a newly staged thirdparty source, but the normal build wrapper can skip refreshing it from older caches.
- Testing: the Japanese analyzer regression exercises real morphology, but the deterministic checks need to use the repository's generated-result regression format.
- Concurrency/locking/persistence: no additional locking, transaction, or persistence issue was found in this static review.
- User focus:
.code-review.d0xn9t/review_focus.txtcontained no additional focus points.
| if (best_prev < 0) { | ||
| return; | ||
| } | ||
| // Search/Extended mode penalizes long compounds so shorter parts win. |
There was a problem hiding this comment.
This penalty changes the primary Viterbi path, so search/extended mode emits only the decompounded path and no longer indexes the original compound token. Lucene/OpenSearch-style search mode normally emits the alternate segmentation in addition to the compound unless an explicit discard-compound option is enabled; this PR exposes no such option, and the tests only assert that 東京 appears. Please either emit both the original compound and decompounded tokens for the default mode, or add an explicit validated discard_compound_token-style property and test both the full-compound and part-token MATCH/TOKENIZE behavior.
| return Status::OK(); | ||
| } | ||
|
|
||
| Status KuromojiDictionary::validate_ranges() const { |
There was a problem hiding this comment.
There are still malformed dictionary shapes that pass this validation and crash or read unrelated metadata later: system.bin can have trie_bytes == 0, leaving the Darts array unset before common_prefix_search() dereferences it; chardef.bin can store category bytes >= CAT_CLASS_COUNT, which is_invoke()/is_group() use to index _defs; and an unknown dictionary can leave zero-count runs, after which the Viterbi safety net emits known=false, word_id=0 and the tokenizer calls unknown_word(0). Please reject these artifacts during load/build, with tests that mutate each case to prove load fails instead of installing a crashable dictionary.
| # `ninja kuromoji_dict` runs the tool over the staged mecab-ipadic source and | ||
| # (re)writes dict/kuromoji/*.bin. Point KUROMOJI_IPADIC_SRC elsewhere if the | ||
| # source is not under the thirdparty share directory. | ||
| set(KUROMOJI_IPADIC_SRC "${THIRDPARTY_DIR}/share/mecab-ipadic-2.7.0-20250920" |
There was a problem hiding this comment.
Making kuromoji_dict part of ALL means a normal non-test BE build now requires ${THIRDPARTY_DIR}/share/mecab-ipadic-2.7.0-20250920, but that directory is only staged when the new mecab_ipadic thirdparty package runs. build.sh still skips build-thirdparty.sh as soon as the old last-library sentinel exists, so an existing thirdparty cache from before this PR can pass the wrapper check and then fail here with a missing IPADIC source. Please extend the thirdparty freshness/pre-build check to require the staged mecab-ipadic directory, or otherwise ensure the default build path refreshes it before CMake runs this target.
| // The assertions below cover the real. | ||
| // Search mode decomposes the compound 東京都 into 東京 + 都, so a 東京 query | ||
| // matches row 1 (a single-character 東 query would NOT, unlike a unigram split). | ||
| def tokyo = sql """ SELECT id FROM ${tableName} WHERE content MATCH '東京' ORDER BY id; """ |
There was a problem hiding this comment.
This new regression is checking deterministic SQL results with manual assertions and also uses def tableName plus a final table drop. The repo regression-test contract asks ordinary single-table tests to hardcode the table name, keep the pre-test drop, preserve the table after the run, and record deterministic results with qt_/order_qt_ plus the generated .out file instead of handwritten assertions. Please convert these result checks to the normal regression format so the Kuromoji coverage is durable in CI output.
| return mode != null ? mode : | ||
| INVERTED_INDEX_PARSER_IK.equals(parser) ? INVERTED_INDEX_PARSER_SMART : | ||
| INVERTED_INDEX_PARSER_COARSE_GRANULARITY; | ||
| INVERTED_INDEX_PARSER_KUROMOJI.equals(parser) ? INVERTED_INDEX_PARSER_KUROMOJI_SEARCH : |
There was a problem hiding this comment.
This adds the Kuromoji default for the parser path, but the PR also makes PROPERTIES("analyzer"="kuromoji") FE-valid via IndexPolicy.BUILTIN_ANALYZERS. That accepted analyzer-only path has no parser key, so this helper still falls through to coarse_grained; BE has the same parser-only defaulting before create_builtin_analyzer(PARSER_KUROMOJI, ...), and kuromoji_mode_from_string("coarse_grained") rejects it during index build/TOKENIZE/MATCH. Please either reject analyzer-only kuromoji and require parser="kuromoji", or make the analyzer key participate in the Kuromoji mode default/validation and add coverage for omitted and explicit modes on analyzer="kuromoji".
7 participants
What problem does this PR solve?
Issue Number: #64646
Related PR: None
Problem Summary:
Doris has no Japanese-aware tokenizer for the inverted index. Japanese text has no spaces between words, so the existing parsers can't segment it and
MATCH/MATCH_PHRASEon Japanese columns end up with poor recall and precision.This PR adds a built-in
kuromojiparser for Japanese, in the same style as the existing Chinese IK analyzer. It's opt-in per column:After indexing, MATCH, MATCH_PHRASE and TOKENIZE() run against the segmented Japanese terms.
How it works:
be/src/storage/index/inverted/analyzer/kuromoji/, so there's no JVM on the indexing path. KuromojiAnalyzer / KuromojiTokenizer mirror the IK analyzer/tokenizer, with a Viterbi cost-model segmenter over the IPADIC connection-cost matrix.Dictionary source is mecab-ipadic-2.7.0-20070801 (NAIST-2003 license, the same lexicon Lucene kuromoji uses).
Release note
Support Japanese text tokenization in the inverted index via a new kuromoji parser (
PROPERTIES("parser"="kuromoji")), withsearch/normal/extendedmodes.Check List (For Author)
parser="kuromoji".Check List (For Reviewer who merge this PR)