[feature](inverted-index) Add Japanese (Kuromoji) morphological analyzer#64667
[feature](inverted-index) Add Japanese (Kuromoji) morphological analyzer#64667nishant94 wants to merge 14 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
|
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
Since this is modeled after Lucene Kuromoji, have you checked how the Doris implementation performs in practice? A small benchmark for indexing throughput would be helpful. |
There was a problem hiding this comment.
Requesting changes. The main issue is that parser=kuromoji is exposed as a production analyzer, but the default package path does not guarantee the runtime dictionary exists, and the runtime silently falls back to per-codepoint tokenization. That can build/query inverted indexes with semantics different from the documented Kuromoji morphology. I also found dictionary loader validation gaps and FE/BE/TOKENIZE default/validation inconsistencies.
| install(DIRECTORY | ||
| ${BASE_DIR}/dict/kuromoji | ||
| DESTINATION ${OUTPUT_DIR}/dict | ||
| OPTIONAL) |
There was a problem hiding this comment.
[Blocking] This install rule does not guarantee that the runtime Kuromoji dictionary is present. The generated dict/kuromoji/*.bin files are ignored/not committed, kuromoji_build_dict is EXCLUDE_FROM_ALL, and kuromoji_dict is only a manual target. A default package can therefore ship only dict/kuromoji/README.md, while the BE later loads ${inverted_index_dict_path}/kuromoji.
Please make package/install depend on dictionary generation and fail if system.bin, matrix.bin, chardef.bin, and unkdict.bin are missing. OPTIONAL should not hide a missing required analyzer artifact.
There was a problem hiding this comment.
Agreed, shipping a package that only contains the README would be a trap.
I made two changes to handle this:
kuromoji_dictnow builds as part of ALL, so a normal build generatessystem.bin/matrix.bin/chardef.bin/unkdict.binfrom the staged mecab-ipadic.- Dropped
OPTIONALand added an install-time check thatFATAL_ERRORsif any of the four files are missing.
| // Loads (once, process-wide) the IPADIC dictionary from `dictPath`. If it is | ||
| // unavailable the tokenizer degrades to a per-codepoint split (logged), rather | ||
| // than failing index/query. | ||
| void initDict(const std::string& dictPath) override { |
There was a problem hiding this comment.
[Blocking] Missing or corrupt dictionary should not silently fall back for the production kuromoji parser. If indexing runs with dict_ == nullptr, segments are written with per-codepoint tokens; after the dictionary is installed/reloaded, query analyzers can produce real Kuromoji tokens, so old and new segments have different tokenization semantics.
Please fail analyzer creation/index/query with a clear error when the Kuromoji dictionary cannot be loaded. If fallback tokenization is desired, expose it as an explicit parser/mode persisted in index metadata.
There was a problem hiding this comment.
Nice catch, you're right that the silent fallback is dangerous. I removed the fallback entirely, initDict now throws when the dictionary can't be loaded, and the tokenizer throws if it's ever handed a null dict.
| const uint8_t* p = _system_map.data(); | ||
| RETURN_IF_ERROR(check_header(p, _system_map.size(), KMJ_KIND_SYSTEM)); | ||
| KmjSystemHeader s {}; | ||
| std::memcpy(&s, p + sizeof(KmjFileHeader), sizeof(s)); |
There was a problem hiding this comment.
[Blocking] check_header() only validates the common header, but the loader then trusts all sub-header offsets/counts and installs mmap pointers from them. A header-valid but truncated/corrupt *.bin can make these memcpy/pointer assignments and later accessors read past the mmap.
Please validate each sub-header before exposing pointers: sizeof(header + subheader), every offset + count * sizeof(T) range, integer overflow, trie byte alignment, class_count == CAT_CLASS_COUNT, matrix dimensions/cell count, run ranges into entry arrays, feature offsets, and word left/right IDs against matrix bounds. Return Status::Corruption for invalid artifacts.
There was a problem hiding this comment.
Fixed. The loader now validates before exposing any pointer.
| return INVERTED_INDEX_PARSER_SMART; | ||
| } | ||
| if (parser_it->second == INVERTED_INDEX_PARSER_KUROMOJI) { | ||
| return INVERTED_INDEX_PARSER_KUROMOJI_SEARCH; |
There was a problem hiding this comment.
[Major] BE now defaults omitted parser_mode for parser=kuromoji to search, but FE InvertedIndexProperties.getInvertedIndexParserMode() still only special-cases IK and otherwise returns coarse_grained. Match predicate thrift serialization uses the FE helper, so FE and BE disagree on the effective default.
Please update the FE default helper to return search for Kuromoji and add FE coverage for a Kuromoji index/query without an explicit parser_mode.
There was a problem hiding this comment.
Updated, InvertedIndexProperties.getInvertedIndexParserMode() to return search for kuromoji so it matches the BE default, and added FE tests for the kuromoji default (and explicit) mode.
| if (mode == "extended") { | ||
| return KuromojiMode::Extended; | ||
| } | ||
| return KuromojiMode::Search; // default (matches OpenSearch/Lucene) |
There was a problem hiding this comment.
[Major] This silently maps any unknown Kuromoji mode to Search. That makes TOKENIZE(..., '"parser"="kuromoji","parser_mode"="bogus"') accepted and executed as search, while index DDL rejects the same value.
Please make mode parsing return an error/status for unknown values, and apply the same Kuromoji property validation to TOKENIZE as DDL/index creation.
There was a problem hiding this comment.
Fixed. kuromoji_mode_from_string now rejects unknown values with an error (empty/search/normal/extended only). Since TOKENIZE builds its analyzer through the same create_builtin_analyzer path, I now rejects a bogus parser_mode just like DDL does. Aksi added a BE test for that as well.
| sql """ INSERT INTO ${tableName} VALUES (3, "Apache Doris は高速です"); """ | ||
| sql "sync" | ||
|
|
||
| // The kuromoji dictionary is not shipped in the p0 package, so the |
There was a problem hiding this comment.
[Blocking] This regression is proving the fallback path, not the feature added by this PR. Because the p0 package does not ship the dictionary, CI can pass while the real Kuromoji/IPADIC morphology path is never exercised.
Please add a required CI/regression path that generates/ships the dictionary and asserts real morphology behavior, e.g. search-mode decompound (東京都 matching 東京), base-form/POS behavior, and extended-mode unknown-word splitting. Keep fallback coverage separate if fallback remains explicit.
There was a problem hiding this comment.
Right, with the dictionary now shipped and the fallback gone.
There was a problem hiding this comment.
Updated, I rewrote test_japanese_analyzer to assert real morphology instead of unigrams.
nishant94
force-pushed
the
feat/kuromoji-japanese-analyzer
branch
from
f334a15 to
698dfb5
Compare
Add a built-in `kuromoji` inverted-index parser that segments Japanese text into morphemes, mirroring the existing Chinese IK analyzer.
- Added `darts.h` to `.clang-format-ignore` and `.licenserc.yaml`. - Improved code formatting in various Kuromoji source files for better readability. - Updated tests files to include necessary headers.
…mposition - Added support for search mode in the Kuromoji Viterbi segmenter, applying penalties for long all-kanji and other tokens to enhance search recall. - Updated the KuromojiMode enumeration to reflect the new search and extended modes. - Modified the KuromojiTokenizer to utilize the new mode functionality. - Added unit tests to validate the behavior of the search mode, ensuring correct segmentation of compounds. - Updated NOTICE.txt to include Apache Lucene as a dependency for the kuromoji analyzer.
…wn words - Implemented functionality in the Kuromoji Viterbi segmenter to decompose unknown (out-of-vocabulary) words into per-character unigrams when in extended mode, aligning with Lucene's JapaneseTokenizer behavior. - Added unit tests to validate the correct segmentation of unknown words in both normal and extended modes, ensuring expected outputs for various input scenarios.
- 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 !! |
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)