github-actions[bot] commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4061837464
##########
be/src/storage/index/inverted/token_filter/token_filter.h:
##########
@@ -28,9 +28,32 @@ class DorisTokenFilter : public TokenFilter, public
DorisTokenStream {
void reset() override { _in->reset(); }
+ std::span<const int32_t> get_source_byte_offsets() const override {
Review Comment:
[P1] Preserve provenance across ASCII-fold expansions. This generic
delegation assumes a filter keeps the upstream rune layout, but a valid
`keyword -> asciifolding -> pinyin(ignore_pinyin_offset=false)` chain violates
that assumption. For `ꜳ刘`, Keyword publishes source boundaries `[0,3,6]`; ASCII
folding rewrites it to `aa刘` while forwarding the three-entry map. Pinyin now
sees three rewritten runes, rejects the size-mismatched map, and falls back to
rewritten bytes, so `liu` is `[2,5)` and whole-token outputs end at 5 instead
of source `[3,6)` / 6. This is separate from the WordDelimiter and
ICU-normalizer threads because those filters override provenance only for their
own rewrites; `lowercase` has the same expansion class. Please remap these
transforming filters or publish a conservative source span, with expansion and
reset/reuse coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/invertedindex/AnalyzerIdentityBuilder.java:
##########
@@ -150,47 +204,80 @@ private static String
buildIdentityFromPolicyProperties(IndexPolicyTypeEnum type
* Resolve a component (tokenizer) to its identity.
*/
private static String resolveComponentIdentity(String name,
IndexPolicyTypeEnum expectedType) {
+ return resolveComponentIdentity(name, expectedType, false);
+ }
+
+ private static String resolveComponentIdentity(
+ String name, IndexPolicyTypeEnum expectedType, boolean
lowercaseDownstream) {
if (Strings.isNullOrEmpty(name)) {
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;
- }
- if (policy.isInvalid()) {
- return "invalid-policy:" + policy.getId() + ":" +
policy.getName();
+ if (env != null && env.getIndexPolicyMgr() != null) {
+ IndexPolicy policy =
env.getIndexPolicyMgr().getPolicyByName(name);
+ if (policy != null && policy.getType() == expectedType) {
+ if (policy.isInvalid()) {
+ return "invalid-policy:" + policy.getId() + ":" +
policy.getName();
+ }
+ Map<String, String> props = policy.getProperties();
+ if (props != null && !props.isEmpty()) {
+ TreeMap<String, String> sortedProps = new
TreeMap<>(props);
+ String type = sortedProps.get(IndexPolicy.PROP_TYPE);
+ String normalizedType =
normalizeBuiltinComponentName(type, expectedType);
+ if (normalizedType != null) {
+ if ("empty".equals(normalizedType)) {
+ return "";
+ }
+ if (sortedProps.size() == 1) {
+ return normalizedType;
+ }
+ sortedProps.put(IndexPolicy.PROP_TYPE,
normalizedType);
+ }
+ if (expectedType == IndexPolicyTypeEnum.TOKENIZER
+ &&
"ngram".equals(sortedProps.get(IndexPolicy.PROP_TYPE))) {
+ // This setting only limits policy creation; it
does not change emitted tokens.
+ sortedProps.remove(PROP_MAX_NGRAM_DIFF);
+ }
+ if (expectedType == IndexPolicyTypeEnum.CHAR_FILTER
+ &&
"char_replace".equals(sortedProps.get(IndexPolicy.PROP_TYPE))) {
+ String replacement =
sortedProps.getOrDefault("replacement", " ");
+ String pattern = canonicalizeCharReplacePattern(
+ sortedProps.get("pattern"), replacement,
lowercaseDownstream);
+ if (pattern.isEmpty()) {
+ return "";
+ }
+ sortedProps.put("pattern", pattern);
+ sortedProps.put("replacement", replacement);
+ }
+ return sortedProps.toString();
Review Comment:
[P1] Canonicalize explicit component defaults before building the identity.
A named Pinyin filter with only `type=pinyin` and one with
`type=pinyin,keep_first_letter=true` are both valid and execute identically
because the factory defaults `keep_first_letter` to true, but the first
resolves to `pinyin` while this raw map becomes `{keep_first_letter=true,
type=pinyin}`. Analyzers that reference those two policies also have different
selector names, so both CREATE and ALTER can admit duplicate runtime-equivalent
indexes. The same mismatch applies to the other optional Pinyin settings (and
other components with defaults). Please canonicalize effective factory
settings, including accepted boolean/integer spellings, and cover
omitted-versus-explicit defaults in identity and CREATE/ALTER tests.
##########
be/src/storage/index/inverted/token_filter/pinyin_filter.cpp:
##########
@@ -214,29 +228,82 @@ bool PinyinFilter::readTerm(Token* token) {
return false;
}
-bool PinyinFilter::processCurrentToken() {
- processed_candidate_ = true;
+bool PinyinFilter::prepareCurrentSource(std::vector<UChar32>&
source_codepoints) {
+ size_t source_start = 0;
+ size_t source_end = current_token_text_.size();
+ if (config_->trimWhitespace) {
+ source_start = current_token_text_.find_first_not_of(" \t\n\r");
+ if (source_start == std::string::npos) {
+ return false;
+ }
+ source_end = current_token_text_.find_last_not_of(" \t\n\r") + 1;
+ }
+ current_source_ = current_token_text_.substr(source_start, source_end -
source_start);
- if (!has_current_token_) {
+ if (current_source_.empty()) {
return false;
}
- current_source_ = current_token_text_;
-
- // Apply trimming if configured
- if (config_->trimWhitespace) {
- current_source_ = trim(current_source_);
+ current_runes_ = convertToRunes(current_source_, source_codepoints);
Review Comment:
[P1] Skip the new provenance arrays when Pinyin offsets are disabled.
`ignore_pinyin_offset` defaults to true, so the factory does not request an
upstream map, but this path still retains a 12-byte `RuneInfo` per rune and
then decodes the same token again into another 12-byte rune vector plus a
4-byte codepoint vector. Together with `source_codepoints`, a 100 MiB ASCII
token reaches roughly 3.2 GB of rune arrays here versus roughly 1.6 GB before
this change, before strings, Pinyin results, candidates, or vector spare
capacity; analyzed values bypass `ignore_above` and the Empty tokenizer is
unbounded. `current_runes_` is only consulted under `!ignorePinyinOffset`, and
the original decode is unnecessary when the exact map is empty. Keep the old
codepoint-only path for the default, decode original runes only when slicing an
exact map, and add large-input/reset allocation coverage.
##########
be/src/storage/index/inverted/similarity/predicate_collector.cpp:
##########
@@ -376,8 +378,9 @@ Status MatchPredicateCollector::collect(RuntimeState*
state, const TabletSchemaS
DORIS_CHECK(analyzer_ctx != nullptr);
const auto query_type = match_query_type(expr->op());
DORIS_CHECK(query_type != InvertedIndexQueryType::UNKNOWN_QUERY);
- const auto* index_meta = DORIS_TRY(select_index_meta(
- candidates.index_metas, candidates.field_type, query_type,
analyzer_ctx->analyzer_key));
+ const auto* index_meta = DORIS_TRY(
Review Comment:
[P1] Convert lazy analyzer failures at the score-collection boundary. After
this selection, line 392 tokenizes through the custom analyzer; on its first IK
use, dictionary initialization can now throw the
`doris::Exception(INVERTED_INDEX_ANALYZER_ERROR)` produced by
`CustomAnalyzer::create_components()`. This collector returns `Status` but does
not catch it, and the downstream collection/scanner chain only uses
`RETURN_IF_ERROR`, so an `ORDER BY score()` MATCH query lets the exception
cross those Status APIs. The analyzed SEARCH branches at lines 529/535 have the
same gap. The earlier fixes cover classic and ARRAY reader/writer boundaries,
not these scoring collectors. Return a Result/Status from query analysis,
convert both CLucene and Doris analyzer exceptions here, and add
missing-dictionary score-runtime coverage.
--
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]
