github-actions[bot] commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4052148488
##########
be/src/storage/index/inverted/analyzer/custom_analyzer.cpp:
##########
@@ -70,12 +70,17 @@ ReaderPtr CustomAnalyzer::init_reader(ReaderPtr reader) {
}
TokenStreamComponentsPtr CustomAnalyzer::create_components() {
- auto tk = _tokenizer->create();
- TokenStreamPtr ts = tk;
- for (const auto& filter : _token_filters) {
- ts = filter->create(ts);
+ try {
+ auto tk = _tokenizer->create();
+ TokenStreamPtr ts = tk;
+ for (const auto& filter : _token_filters) {
+ ts = filter->create(ts);
+ }
+ return std::make_shared<TokenStreamComponents>(tk, ts);
+ } catch (const CLuceneError& e) {
Review Comment:
[P1] Keep lazy IK failures inside the classic Status boundaries
This catch changes a lazy `IKTokenizerFactory::create()` failure from
`CLuceneError` to `doris::Exception`, but the classic call sites still catch
only `CLuceneError`. `InvertedIndexColumnWriter::new_inverted_index_field()`
invokes `reusableTokenStream()` inside a `Status` method, and
`FullTextIndexReader::query()` invokes `get_analyse_result()` inside another
`Status` method; with a missing or corrupt IK dictionary, the converted
exception now escapes both instead of returning their analyzer error. The added
test calls `CustomAnalyzer::tokenStream()` directly, so it does not cover
either boundary. Please preserve the original exception here or teach every
classic invocation boundary to convert `doris::Exception` to a non-OK `Status`,
and add a writer/query failure-path test.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/invertedindex/AnalyzerIdentityBuilder.java:
##########
@@ -226,26 +310,81 @@ private static String resolveTokenFilterIdentity(String
filterList) {
* IMPORTANT: Order is preserved because filter order is semantically
significant.
*/
private static String resolveCharFilterIdentity(String filterList) {
+ return resolveCharFilterIdentity(filterList, false);
+ }
+
+ private static String resolveCharFilterIdentity(String filterList, boolean
lowercaseIk) {
if (Strings.isNullOrEmpty(filterList)) {
return "";
}
- StringBuilder sb = new StringBuilder();
+ ArrayDeque<String> identities = new ArrayDeque<>();
String[] filters = filterList.split(",\\s*");
// DO NOT sort - filter order is semantically significant
- for (int i = 0; i < filters.length; i++) {
- String filter = filters[i].trim();
- if (i > 0) {
- sb.append(",");
+ for (int i = filters.length - 1; i >= 0; --i) {
+ String filter = resolveComponentIdentity(filters[i].trim(),
IndexPolicyTypeEnum.CHAR_FILTER, lowercaseIk);
+ if (Strings.isNullOrEmpty(filter)) {
+ continue;
}
+ identities.addFirst(filter);
+ // An earlier replacement can change the input of a later filter.
+ lowercaseIk = false;
Review Comment:
[P1] Preserve absorption through a downstream case-folding filter
Clearing `lowercaseIk` after every nonempty downstream filter still gives
different identities to equivalent pipelines. For example, let
`lower_a={type=char_replace,pattern=A,replacement=a}` and
`fold={type=icu_normalizer}` with its default `nfkc_cf`;
`{tokenizer=ik_smart,char_filter=fold}` and
`{tokenizer=ik_smart,char_filter=lower_a,fold}` emit the same text, positions,
and source offsets because `nfkc_cf` already maps `A` to `a`. Walking backward
resolves `fold`, clears the absorption context here, and then retains
`lower_a`, so CREATE/ALTER can admit both indexes. Please propagate the known
`nfkc_cf` case-folding context backward (or canonicalize the composed pipeline)
and cover this intervening-filter case.
--
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]
