github-actions[bot] commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4000457870
##########
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] Make the new built-in names locale-independent
`IndexPolicyMgr.normalizeKey()` still calls parameterless `toLowerCase()`.
On a Turkish-locale FE, `IK_SMART` becomes `ık_smart` (dotless i), misses this
set, and the analyzer DDL is rejected even though the new identity path uses
`Locale.ROOT` and the feature is intended to be case-insensitive.
`AnalyzerKeyNormalizer` has the same default-locale conversion, and startup
does not pin the JVM locale. Please use `Locale.ROOT` consistently in
validation/property normalization and cover uppercase IK through FE validation
under a Turkish default locale.
##########
be/test/runtime/index_policy/index_policy_mgr_test.cpp:
##########
@@ -185,6 +185,57 @@ TEST_F(IndexPolicyMgrTest, TestTokenFilterProcessing) {
ASSERT_NE(emptyAnalyzer, nullptr);
}
+TEST_F(IndexPolicyMgrTest, BuiltinTokenizerNamesAreCaseInsensitive) {
+ const char* doris_home = std::getenv("DORIS_HOME");
+ ASSERT_NE(doris_home, nullptr);
+ config::inverted_index_dict_path = std::string(doris_home) + "../../dict";
+
+ TIndexPolicy analyzer;
+ analyzer.id = 20;
+ analyzer.name = "uppercase_ik_analyzer";
+ analyzer.type = TIndexPolicyType::ANALYZER;
+ analyzer.properties["tokenizer"] = "IK_SMART";
+ mgr.apply_policy_changes({analyzer}, {});
+
+ auto built = mgr.get_policy_by_name(analyzer.name);
+ ASSERT_NE(built, nullptr);
+ auto reader =
segment_v2::inverted_index::InvertedIndexAnalyzer::create_reader({});
+ const std::string text = "我来到北京";
+ reader->init(text.data(), static_cast<int32_t>(text.size()), false);
+ auto terms =
segment_v2::inverted_index::InvertedIndexAnalyzer::get_analyse_result(reader,
Review Comment:
[P1] Keep the combined BE test run independent
This new test reaches `IKTokenizerFactory::create()` and successfully
consumes `Dictionary`'s process-wide `once_flag`. In the same `doris_be_test`
binary, `IKTokenizerTest.TestDictionaryExceptionHandling` later expects its
invalid-path call to be the first `Dictionary::initial()` and therefore to
throw; after this test it simply returns the existing singleton instead. The PR
validation already reports that combined-suite failure and only an isolated IK
rerun passing. Please make the dictionary failure coverage
process-isolated/resettable or avoid initializing it here (and restore
`inverted_index_dict_path`), then verify the suites together; BE unit tests
must not depend on execution order.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/invertedindex/AnalyzerIdentityBuilder.java:
##########
@@ -152,35 +153,29 @@ private static String resolveComponentIdentity(String
name, IndexPolicyTypeEnum
return "";
}
- // Check if it's a built-in component
- if (expectedType == IndexPolicyTypeEnum.TOKENIZER
- && IndexPolicy.BUILTIN_TOKENIZERS.contains(name)) {
- return name;
- }
-
- // For custom component, get its properties
+ // Existing named policies take precedence over built-ins for upgrade
compatibility.
try {
Env env = Env.getCurrentEnv();
- if (env == null || env.getIndexPolicyMgr() == null) {
- return name;
- }
-
- IndexPolicy policy = env.getIndexPolicyMgr().getPolicyByName(name);
- if (policy == null || policy.getType() != expectedType) {
- return name;
- }
-
- Map<String, String> props = policy.getProperties();
- if (props == null || props.isEmpty()) {
- return name;
+ if (env != null && env.getIndexPolicyMgr() != null) {
+ IndexPolicy policy =
env.getIndexPolicyMgr().getPolicyByName(name);
+ if (policy != null && policy.getType() == expectedType) {
+ Map<String, String> props = policy.getProperties();
+ if (props != null && !props.isEmpty()) {
+ TreeMap<String, String> sortedProps = new
TreeMap<>(props);
+ return sortedProps.toString();
+ }
+ }
}
-
- // Build identity from sorted properties
- TreeMap<String, String> sortedProps = new TreeMap<>(props);
- return sortedProps.toString();
} catch (RuntimeException e) {
- return name;
+ // Fall through to built-in resolution or the original name.
+ }
+
+ String normalizedName = name.toLowerCase(Locale.ROOT);
Review Comment:
[P1] Canonicalize built-in filter names in analyzer identity
Validation and the changed BE lookup accept built-in token/character filter
names case-insensitively, but these resolvers recognize their built-in sets
case-sensitively and the fallback canonicalizes only `TOKENIZER`. As a result,
policies using `pinyin` and `PINYIN` build the same analyzer but receive
different identities, so duplicate-index checks can treat equivalent analyzers
as distinct. Please normalize built-in token-filter and char-filter names
before identity construction and cover case-variant policies in the identity
tests.
##########
be/src/storage/index/inverted/token_filter/pinyin_filter.cpp:
##########
@@ -421,8 +423,18 @@ void PinyinFilter::setTokenAttributes(Token* token, const
std::string& term, int
int end_offset, int position) {
set_text(token, term);
- token->setStartOffset(start_offset);
- token->setEndOffset(end_offset);
+ int absolute_start = current_start_offset_;
+ int absolute_end = current_end_offset_;
+ const bool is_whole_token =
+ start_offset == 0 && std::cmp_equal(end_offset,
current_source_.length());
Review Comment:
[P1] Preserve source-byte boundaries through IK normalization
`current_runes_` is built from the token text after `IKTokenizer::next()`
has applied `regularizeString()`, so these byte offsets no longer describe the
original source whenever normalization changes width. For example, IK records
`ＬＩＵＤＥ` with source span `[0,15)`, normalizes it to `liude`, and the default
pinyin alphabet split emits `liu`/`de`; this code reports `[0,3)`/`[3,5)`
instead of `[0,9)`/`[9,15)`. The new CJK-only offset test keeps rune widths
unchanged and misses this. Please carry an original-byte boundary map through
IK normalization (or equivalent source mapping) and cover a split full-width
token.
##########
fe/fe-core/src/main/java/org/apache/doris/indexpolicy/IndexPolicyMgr.java:
##########
@@ -373,6 +374,10 @@ private void validateTokenizerProperties(Map<String,
String> properties) throws
case "basic":
validator = new BasicTokenizerValidator();
Review Comment:
[P1] Give type-only IK wrappers the direct built-in identity
These cases now allow a named tokenizer policy whose only property is
`type=ik_smart` or `type=ik_max_word`. `AnalyzerIdentityBuilder` represents
that wrapper as `{type=ik_smart}`, while a direct reference is `ik_smart`; BE
removes the `type` property and sends the same factory with empty settings in
both cases. The two duplicate-index checks therefore accept indexes whose
effective analyzers are identical. Please base identity on the resolved factory
plus settings (collapsing a type-only wrapper to the direct built-in identity)
and test direct versus named IK policies in both duplicate-index paths.
--
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]
