airborne12 commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4081902606
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/invertedindex/AnalyzerIdentityBuilder.java:
##########
@@ -45,14 +113,61 @@ public static String buildAnalyzerIdentity(
}
if (!Strings.isNullOrEmpty(preferredAnalyzer)) {
+ String builtinIkIdentity =
resolveBuiltinIkAnalyzerIdentity(properties, preferredAnalyzer);
+ if (builtinIkIdentity != null) {
+ return appendOuterCharFilterIdentity(
+ builtinIkIdentity, properties,
builtinIkFoldContext(builtinIkIdentity));
+ }
// For custom analyzer/normalizer, resolve to underlying config to
build identity
- return resolveAnalyzerIdentity(preferredAnalyzer,
defaultAnalyzerKey, log);
+ return appendOuterCharFilterIdentity(
+ resolveAnalyzerIdentity(preferredAnalyzer,
defaultAnalyzerKey, log), properties,
Review Comment:
Confirmed and fixed in 9137635108c.
`create_builtin_analyzer()` builds `BasicAnalyzer` and `ICUAnalyzer`, and
both wrap their tokenizer in a `LowerCaseFilter` (`basic_analyzer.h:70`). The
tokenizers match the custom path exactly: `BasicTokenizer::initialize()` gets
the same empty `extra_chars`, and ICU is initialized with the same
`config::inverted_index_dict_path + "/icu"` expression the custom factory uses.
Both built-ins take the identity of that pipeline now, and `unicode` is handled
in the other thread.
Two details we checked before merging them:
`lower_case=false` is a real fork rather than noise. `set_lowercase()` is
called only from the built-in dispatch (`analyzer.cpp:153,155`), and
`CustomAnalyzer` never reads `_lowercase`, so a built-in with
`lower_case=false` has no LowerCaseFilter and is equivalent to a custom
analyzer with the tokenizer alone. The identity maps it that way instead of
dropping the property.
`stopwords` and `parser_mode` are inert for these two. `_stopwords` is read
only by `standard95::StandardAnalyzer` (`StandardAnalyzer.h:19,25`) and neither
analyzer's `create_components()` looks at it, and the BASIC and ICU branches do
not read `parser_mode`. Tests assert that neither property splits the identity.
The identity is spelled out rather than resolved through the component
lookup, because `is_builtin_analyzer()` intercepts these names before the
policy manager, so the built-in keeps its own tokenizer even if a policy
shadows the name.
One thing this surfaced that the thread did not mention: the built-ins were
not handing the downstream LowerCaseFilter's fold context to the outer char
filter, so an outer `char_replace A -> a` was absorbed on the custom side but
not on the built-in side. Without fixing that, only the no-char-filter half
would have merged.
Tests: `testBuiltinBasicAndIcuTakeTheirLowercasePipelineIdentity` and
`testBuiltinLowercaseAnalyzerAbsorbsOuterCharFilterLikeItsPipeline`, plus
`testCreateTableRejectsBuiltinAnalyzerPipelineAliases` and
`testAddInvertedIndexRejectsBuiltinAnalyzerPipelineAliases` for the two DDL
paths. Negatives keep `lower_case=false` apart from the default, basic apart
from icu, and a custom `extra_chars` apart from the bare tokenizer.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/invertedindex/AnalyzerIdentityBuilder.java:
##########
@@ -45,14 +113,61 @@ public static String buildAnalyzerIdentity(
}
if (!Strings.isNullOrEmpty(preferredAnalyzer)) {
+ String builtinIkIdentity =
resolveBuiltinIkAnalyzerIdentity(properties, preferredAnalyzer);
+ if (builtinIkIdentity != null) {
+ return appendOuterCharFilterIdentity(
+ builtinIkIdentity, properties,
builtinIkFoldContext(builtinIkIdentity));
+ }
// For custom analyzer/normalizer, resolve to underlying config to
build identity
- return resolveAnalyzerIdentity(preferredAnalyzer,
defaultAnalyzerKey, log);
+ return appendOuterCharFilterIdentity(
+ resolveAnalyzerIdentity(preferredAnalyzer,
defaultAnalyzerKey, log), properties,
+ customAnalyzerFoldContext(preferredAnalyzer));
}
if (Strings.isNullOrEmpty(parser) ||
parserNone.equalsIgnoreCase(parser)) {
return defaultAnalyzerKey;
}
- return parser;
+ String legacyIkIdentity = resolveLegacyIkIdentity(properties, parser);
+ if (legacyIkIdentity != null) {
+ return appendOuterCharFilterIdentity(
+ legacyIkIdentity, properties,
builtinIkFoldContext(legacyIkIdentity));
+ }
+ return appendOuterCharFilterIdentity(parser, properties, null);
Review Comment:
Confirmed and fixed in 9137635108c.
`create_builtin_analyzer()` takes `PARSER_STANDARD` and `PARSER_UNICODE` in
one branch and constructs the same `standard95::StandardAnalyzer` for both,
with no per-type detail afterwards; `set_lowercase()` and `set_stopwords()`
treat them alike. The identity now maps `unicode` onto `standard`, for both the
`analyzer=` and the `parser=` spellings.
While checking this we noticed the same problem one level down:
`get_inverted_index_parser_type_from_string()` matches on `parser_str_lower`,
so `parser=STANDARD` reaches the same analyzer as `parser=standard`, but the
identity kept the spelling and let the pair through the fences. The parser name
is folded now too, with assertions for `STANDARD`, `Unicode` and `English`.
Test: `testUnicodeAndStandardShareOneBuiltinIdentity`, covering both
spellings, `lower_case=false`, `stopwords=none` and an outer char filter, with
`english`, `chinese` and `basic` as negatives.
--
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]
