github-actions[bot] commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4057324012


##########
be/src/storage/index/inverted/token_filter/pinyin_filter.cpp:
##########
@@ -79,6 +80,11 @@ Token* PinyinFilter::next(Token* token) {
         current_token_text_ = std::string(token->termBuffer<char>(), 
token->termLength<char>());
         current_start_offset_ = token->startOffset();
         current_end_offset_ = token->endOffset();
+        auto source_byte_offsets = get_source_byte_offsets();
+        current_source_byte_offsets_.assign(source_byte_offsets.begin(), 
source_byte_offsets.end());

Review Comment:
   This source-map snapshot is empty for several FE-valid upstream tokenizers 
(`basic`, `icu`, `char_group`, `ngram`, `empty`): their `next()` methods still 
call `set`/`setNoCopy` without publishing byte boundaries. With 
`pinyin(ignore_pinyin_offset=false)`, Basic tokenizes `刘德华` as three `[0,len)` 
tokens, so the Pinyin alternatives for later characters reuse local offsets 
instead of document spans; the same happens after reset/multiple tokens. Please 
make every supported upstream publish corrected source boundaries when offset 
tracking is enabled, or reject this Pinyin setting for unsupported tokenizers, 
and add a multi-token regression beyond Keyword/Standard.



##########
fe/fe-core/src/main/java/org/apache/doris/analysis/InvertedIndexUtil.java:
##########
@@ -423,17 +445,27 @@ public static boolean isAnalyzerMatched(Map<String, 
String> properties, String a
                     buildAnalyzerIdentity(properties));
         }
 
+        String resolvedAnalyzer = resolveAnalyzerName(normalizedAnalyzer);
         String preferredAnalyzer = 
InvertedIndexProperties.getPreferredAnalyzer(properties);
         if (!Strings.isNullOrEmpty(preferredAnalyzer)) {
-            return normalizedAnalyzer.equalsIgnoreCase(preferredAnalyzer);

Review Comment:
   Two valid non-IK indexes can now differ only by the outer `char_replace` 
settings because `AnalyzerIdentityBuilder` appends those settings to the 
identity, but this matcher still compares only the resolved analyzer name. For 
example, `standard` with `a->b` and `standard` with `x->y` both satisfy this 
branch; `OlapTable.getInvertedIndex(..., "standard")` then keeps both and 
chooses the first analyzed index. `MatchPredicate` serializes that arbitrary 
index's char-filter map, so `USING ANALYZER standard` can analyze with the 
wrong replacement and the other index is unreachable. Please carry the 
effective outer-filter identity into FE selection (or reject same-name indexes 
that cannot be disambiguated by the query syntax) and add a regression proving 
the selected Thrift properties.



-- 
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]

Reply via email to