github-actions[bot] commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4000130292
##########
be/src/storage/index/inverted/analyzer/ik/IKTokenizer.cpp:
##########
@@ -31,27 +31,36 @@ Token* IKTokenizer::next(Token* token) {
return nullptr;
}
- std::string& token_text = tokens_text_[buffer_index_++];
+ TokenData& token_data = tokens_[buffer_index_++];
// full-width to half-width, and lowercase
// TODO(ryan19929): do regularizeString in fillBuffer.
- CharacterUtil::regularizeString(token_text, this->lowercase);
- size_t size = std::min(token_text.size(),
static_cast<size_t>(LUCENE_MAX_WORD_LEN));
- token->setNoCopy(token_text.data(), 0, static_cast<int32_t>(size));
+ CharacterUtil::regularizeString(token_data.text, this->lowercase);
+ size_t size = std::min(token_data.text.size(),
static_cast<size_t>(LUCENE_MAX_WORD_LEN));
+ set(token, std::string_view(token_data.text.data(), size));
+ token->setStartOffset(token_data.start_offset);
+ token->setEndOffset(token_data.end_offset);
return token;
}
+void IKTokenizer::reset() {
+ inverted_index::DorisTokenizer::reset();
+ reset(_in.get());
+}
+
void IKTokenizer::reset(lucene::util::Reader* reader) {
this->input = reader;
this->buffer_index_ = 0;
this->data_length_ = 0;
- this->tokens_text_.clear();
+ this->tokens_.clear();
try {
buffer_.reserve(input->size());
ik_segmenter_->reset(reader);
Lexeme lexeme;
while (ik_segmenter_->next(lexeme)) {
- tokens_text_.emplace_back(lexeme.getText());
+ tokens_.push_back({lexeme.getText(),
+
static_cast<int32_t>(lexeme.getByteBeginPosition()),
Review Comment:
[P1] Fix the absolute base before publishing long-input offsets
After the first 4096-byte refill, these values are already too small.
`AnalyzeContext::fillBuffer()` carries bytes beginning at
`typed_runes_[cursor_].getNextBytePosition()`, but `markBufferOffset()`
advances `buffer_offset_` only by `typed_runes_[cursor_].offset`. Because the
cursor rune was analyzed and then dropped by the memmove, the next buffer is
based at its end while the recorded absolute base still points at its start;
every later `Lexeme` offset is short by that rune's byte length, cumulatively
across refills. Please advance by the same consumed-byte boundary and add exact
offset checks on both sides of a multibyte refill.
##########
be/src/storage/index/inverted/analyzer/ik/IKTokenizer.cpp:
##########
@@ -31,27 +31,36 @@ Token* IKTokenizer::next(Token* token) {
return nullptr;
}
- std::string& token_text = tokens_text_[buffer_index_++];
+ TokenData& token_data = tokens_[buffer_index_++];
// full-width to half-width, and lowercase
// TODO(ryan19929): do regularizeString in fillBuffer.
- CharacterUtil::regularizeString(token_text, this->lowercase);
- size_t size = std::min(token_text.size(),
static_cast<size_t>(LUCENE_MAX_WORD_LEN));
- token->setNoCopy(token_text.data(), 0, static_cast<int32_t>(size));
+ CharacterUtil::regularizeString(token_data.text, this->lowercase);
+ size_t size = std::min(token_data.text.size(),
static_cast<size_t>(LUCENE_MAX_WORD_LEN));
+ set(token, std::string_view(token_data.text.data(), size));
+ token->setStartOffset(token_data.start_offset);
+ token->setEndOffset(token_data.end_offset);
return token;
}
+void IKTokenizer::reset() {
Review Comment:
[P0] Preserve the indexing reset contract
CLucene always calls `stream->reset()` immediately before consuming a
tokenized field, but this new eager reset breaks both IK entry paths. Legacy
`IKAnalyzer` already called `reset(reader)` and filled tokens without setting
`DorisTokenizer::_in_pending`; CLucene's next call now promotes null and
`reset(_in.get())` reaches `input->size()`, crashing ordinary existing
`parser=ik` writes. For custom IK ARRAY elements,
`CustomAnalyzer::tokenStream()` performs the first eager reset, then CLucene's
second reset clears those tokens and re-reads the same reader at EOF, so the
element indexes no terms. Please make CLucene's reset-before-consume the single
tokenization point (while preserving raw-reader compatibility), and cover
legacy IK IndexWriter plus custom IK ARRAY indexing.
##########
fe/fe-core/src/main/java/org/apache/doris/indexpolicy/IndexPolicy.java:
##########
@@ -59,7 +59,8 @@ public class IndexPolicy implements Writable,
GsonPostProcessable {
public static final String PROP_TOKEN_FILTER = "token_filter";
public static final String PROP_CHAR_FILTER = "char_filter";
public static final Set<String> BUILTIN_TOKENIZERS = ImmutableSet.of(
- "empty", "ngram", "edge_ngram", "keyword", "standard",
"char_group", "basic", "icu", "pinyin");
+ "empty", "ngram", "edge_ngram", "keyword", "standard",
"char_group", "basic", "icu", "pinyin",
+ "ik_smart", "ik_max_word");
Review Comment:
[P1] Accept the IK types in TOKENIZER policy validation
Adding these names to `BUILTIN_TOKENIZERS` makes FE advertise them as
supported tokenizer types, but `validateTokenizerProperties()` still has no
`ik_smart`/`ik_max_word` case. `CREATE INVERTED INDEX TOKENIZER t PROPERTIES
("type"="ik_smart")` therefore reaches the default branch and is rejected with
an error whose own supported-types list includes `ik_smart`; the max-word form
behaves the same. BE can resolve both new types if they reach it, and every
previous built-in has a validator case. Please add type-only validation for
both modes (and cover a named tokenizer policy), or separate direct built-ins
from creatable/advertised types.
##########
regression-test/suites/inverted_index_p0/analyzer/test_ik_custom_analyzer.groovy:
##########
@@ -0,0 +1,123 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_ik_custom_analyzer", "p0") {
+ def pinyinFilter = "test_ik_pinyin_filter"
+ def smartAnalyzer = "test_ik_smart_pinyin_analyzer"
+ def maxWordAnalyzer = "test_ik_max_word_pinyin_analyzer"
+ def tableName = "test_ik_custom_analyzer"
Review Comment:
[P2] Capture these deterministic results as regression goldens
This suite uses `def tableName` for its sole ordinary test table and checks
every deterministic `TOKENIZE`/`MATCH` result via `assertTrue`/`assertEquals`,
so it produces no generated `.out` evidence. The repository regression rules
require a hardcoded name for a single ordinary table and `qt_`/`order_qt_`
output for determined results. Please convert these checks to named golden
queries (keeping `ORDER BY` for row results), generate the `.out` through the
runner, and hardcode the table name.
##########
regression-test/suites/inverted_index_p0/analyzer/test_ik_custom_analyzer.groovy:
##########
@@ -0,0 +1,123 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_ik_custom_analyzer", "p0") {
+ def pinyinFilter = "test_ik_pinyin_filter"
+ def smartAnalyzer = "test_ik_smart_pinyin_analyzer"
+ def maxWordAnalyzer = "test_ik_max_word_pinyin_analyzer"
+ def tableName = "test_ik_custom_analyzer"
+
+ sql "DROP TABLE IF EXISTS ${tableName}"
+ try_sql "DROP INVERTED INDEX ANALYZER IF EXISTS ${smartAnalyzer}"
+ try_sql "DROP INVERTED INDEX ANALYZER IF EXISTS ${maxWordAnalyzer}"
+ try_sql "DROP INVERTED INDEX TOKEN_FILTER IF EXISTS ${pinyinFilter}"
+
+ sql """
+ CREATE INVERTED INDEX TOKEN_FILTER IF NOT EXISTS ${pinyinFilter}
+ PROPERTIES (
+ "type" = "pinyin",
+ "keep_none_chinese" = "false",
+ "keep_first_letter" = "true",
+ "keep_full_pinyin" = "false",
+ "keep_separate_first_letter" = "false",
+ "keep_original" = "true",
+ "keep_joined_full_pinyin" = "true"
+ )
+ """
+ sql """
+ CREATE INVERTED INDEX ANALYZER IF NOT EXISTS ${smartAnalyzer}
+ PROPERTIES (
+ "tokenizer" = "ik_smart",
+ "token_filter" = "${pinyinFilter}"
+ )
+ """
+ sql """
+ CREATE INVERTED INDEX ANALYZER IF NOT EXISTS ${maxWordAnalyzer}
+ PROPERTIES (
+ "tokenizer" = "ik_max_word",
+ "token_filter" = "${pinyinFilter}"
+ )
+ """
+
+ def waitAnalyzerReady = { analyzerName ->
+ int maxRetry = 30
+ Exception lastException = null
+ for (int i = 0; i < maxRetry; i++) {
+ try {
+ sql """SELECT TOKENIZE('probe',
'"analyzer"="${analyzerName}"')"""
+ return
+ } catch (Exception e) {
+ lastException = e
+ sleep(1000)
+ }
+ }
+ assertTrue(false, "Analyzer ${analyzerName} was not ready:
${lastException?.message}")
+ }
+
+ waitAnalyzerReady(smartAnalyzer)
+ waitAnalyzerReady(maxWordAnalyzer)
+
+ def smartTokens = sql """SELECT TOKENIZE('我来到北京清华大学',
'"analyzer"="${smartAnalyzer}"')"""
+ def smartTokenString = smartTokens[0][0].toString()
+ assertTrue(smartTokenString.contains('"token": "清华大学"'))
+ assertTrue(smartTokenString.contains('"token": "qinghuadaxue"'))
+
+ def maxWordTokens = sql """SELECT TOKENIZE('我来到北京清华大学',
'"analyzer"="${maxWordAnalyzer}"')"""
+ def maxWordTokenString = maxWordTokens[0][0].toString()
+ assertTrue(maxWordTokenString.contains('"token": "清华"'))
+ assertTrue(maxWordTokenString.contains('"token": "qinghua"'))
+
+ sql """
+ CREATE TABLE ${tableName} (
+ id INT,
+ content STRING,
+ INDEX idx_smart (content) USING INVERTED
+ PROPERTIES("analyzer" = "${smartAnalyzer}", "support_phrase" =
"true"),
+ INDEX idx_max_word (content) USING INVERTED
+ PROPERTIES("analyzer" = "${maxWordAnalyzer}", "support_phrase"
= "true")
Review Comment:
[P1] Make the enabled phrase paths handle pinyin alternatives
This index is marked `support_phrase=true`, but the configured filter emits
清华大学, `qinghuadaxue`, and `qhdx` at the same token position (increments 1,0,0).
Legacy CLucene assigns those separate `TermInfo`s positions by vector ordinal,
and SNII likewise flattens them into a sequential term vector, so an exact
`MATCH_PHRASE` requires the alternatives consecutively and misses the indexed
row. The query-v2 CLucene path already groups equal-position terms before
selecting `PhraseQuery`/`MultiPhraseQuery`. Please apply equivalent grouping to
both direct readers and add `MATCH_PHRASE` coverage here; the suite currently
exercises only `MATCH`.
##########
be/src/storage/index/inverted/analyzer/ik/IKTokenizer.cpp:
##########
@@ -31,27 +31,36 @@ Token* IKTokenizer::next(Token* token) {
return nullptr;
}
- std::string& token_text = tokens_text_[buffer_index_++];
+ TokenData& token_data = tokens_[buffer_index_++];
// full-width to half-width, and lowercase
// TODO(ryan19929): do regularizeString in fillBuffer.
- CharacterUtil::regularizeString(token_text, this->lowercase);
- size_t size = std::min(token_text.size(),
static_cast<size_t>(LUCENE_MAX_WORD_LEN));
- token->setNoCopy(token_text.data(), 0, static_cast<int32_t>(size));
+ CharacterUtil::regularizeString(token_data.text, this->lowercase);
+ size_t size = std::min(token_data.text.size(),
static_cast<size_t>(LUCENE_MAX_WORD_LEN));
+ set(token, std::string_view(token_data.text.data(), size));
+ token->setStartOffset(token_data.start_offset);
Review Comment:
[P1] Preserve IK offsets through the pinyin filter
These absolute offsets are immediately lost in the composition this PR adds:
`PinyinFilter` records the input token's
`current_start_offset_`/`current_end_offset_`, but builds each candidate from
zero and writes those token-local values directly. For smart tokens such as 我
(0-3), 来到 (3-9), 北京 (9-15), filtered outputs restart at 0 for every source
token, so offsets become non-monotonic and no longer refer to the document.
Please rebase candidate offsets on the input token span (and use the full input
span for whole-token alternatives), then add exact multi-token/multibyte offset
assertions; the new tests currently collect only term strings.
##########
fe/fe-core/src/main/java/org/apache/doris/indexpolicy/IndexPolicy.java:
##########
@@ -59,7 +59,8 @@ public class IndexPolicy implements Writable,
GsonPostProcessable {
public static final String PROP_TOKEN_FILTER = "token_filter";
public static final String PROP_CHAR_FILTER = "char_filter";
public static final Set<String> BUILTIN_TOKENIZERS = ImmutableSet.of(
- "empty", "ngram", "edge_ngram", "keyword", "standard",
"char_group", "basic", "icu", "pinyin");
+ "empty", "ngram", "edge_ngram", "keyword", "standard",
"char_group", "basic", "icu", "pinyin",
+ "ik_smart", "ik_max_word");
Review Comment:
[P1] Define an upgrade rule for existing policies with these names
Before this change, users could legally persist an index policy named
`ik_smart` or `ik_max_word`. Replay/image loading keeps such policies, but
after adding the names here FE validates the spelling as the new built-in
before consulting the stored policy, while BE gives its `_name_to_id` entry
precedence. An old TOKENIZER policy named `ik_smart` (for example
`type=standard`) will therefore shadow the new IK tokenizer on BE even though
FE/identity treat it as built-in; a collision of another policy type can make
later analyzer materialization fail. Please make collision precedence
consistent and migration-safe (for example preserve an existing policy on both
sides and reserve the name only for new DDL), with replay coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/indexpolicy/IndexPolicy.java:
##########
@@ -59,7 +59,8 @@ public class IndexPolicy implements Writable,
GsonPostProcessable {
public static final String PROP_TOKEN_FILTER = "token_filter";
public static final String PROP_CHAR_FILTER = "char_filter";
public static final Set<String> BUILTIN_TOKENIZERS = ImmutableSet.of(
- "empty", "ngram", "edge_ngram", "keyword", "standard",
"char_group", "basic", "icu", "pinyin");
+ "empty", "ngram", "edge_ngram", "keyword", "standard",
"char_group", "basic", "icu", "pinyin",
+ "ik_smart", "ik_max_word");
Review Comment:
[P1] Canonicalize case-insensitive built-in references before BE lookup
FE normalizes a tokenizer reference before checking this set, so
`tokenizer=IK_SMART` is accepted and persisted, but BE passes that original
spelling to `AnalysisFactoryMgr` after only using a normalized copy to look for
a named policy. The factory registry contains only lowercase
`ik_smart`/`ik_max_word` keys, so `TOKENIZE`, index construction, or query
analysis later fails with `Unknown factory name`. Please persist or resolve the
canonical built-in spelling consistently (and use the same canonicalization in
analyzer identity); the new tests currently cover lowercase only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
