github-actions[bot] commented on code in PR #66872:
URL: https://github.com/apache/doris/pull/66872#discussion_r3840617728
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CheckScoreUsage.java:
##########
@@ -133,31 +133,42 @@ private static void
checkSelectedIndexPolicyAdmission(Index selectedIndex,
if (selectedIndex == null) {
return;
}
+ boolean isSnii = scan.getTable().getInvertedIndexFileStorageFormat()
+ == TInvertedIndexFileStorageFormat.SNII;
Map<String, String> properties = selectedIndex.getProperties();
- if (properties == null
- || !properties.containsKey(
-
InvertedIndexProperties.INVERTED_INDEX_ANALYZER_NAME_KEY)) {
+ String analyzerName = properties == null ? null : properties.get(
+ InvertedIndexProperties.INVERTED_INDEX_ANALYZER_NAME_KEY);
+ if (analyzerName == null) {
+ if (isSnii) {
+ throw nonScoringSniiIndex(selectedIndex);
+ }
return;
}
- String analyzerName = properties.get(
- InvertedIndexProperties.INVERTED_INDEX_ANALYZER_NAME_KEY);
boolean usesCommonGrams;
try {
usesCommonGrams =
indexPolicyMgr.validateAnalyzerUsesCommonGrams(analyzerName);
} catch (DdlException e) {
throw new AnalysisException("score() cannot use inverted index '"
+ selectedIndex.getIndexName() + "': " + e.getMessage(),
e);
}
- if (usesCommonGrams
- && scan.getTable().getInvertedIndexFileStorageFormat()
- != TInvertedIndexFileStorageFormat.SNII) {
+ if (isSnii && !usesCommonGrams) {
Review Comment:
[P1] Resolve SEARCH admission with the runtime query type
On a field with both a plain and a CommonGrams index, this can validate the
wrong physical index:
```text
TopN(order by s)
Project(score() AS s)
Filter(search('body:EXACT(alpha)'))
Scan(SNII; idx_keyword(body), idx_cg(body, CommonGrams))
```
resolveSelectedInvertedIndex(slot, null) follows OlapTable.getInvertedIndex
and chooses the analyzed idx_cg, so this condition admits the plan. BE
deliberately maps EXACT to EQUAL_QUERY, ignores the binding analyzer key for
equality, and select_best_inverted_index_candidate prefers the untokenized
STRING_TYPE index; the existing test_search_exact_multi_index.groovy regression
codifies that routing. Scoring then opens idx_keyword, which lacks SNII scoring
metadata, and fails after FE admission. Please resolve admission per
score-bearing SEARCH leaf with the same query-type/index preference as BE, and
cover EXACT plus ANY/ALL on a multi-index SNII field.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CheckScoreUsage.java:
##########
@@ -133,31 +133,42 @@ private static void
checkSelectedIndexPolicyAdmission(Index selectedIndex,
if (selectedIndex == null) {
return;
}
+ boolean isSnii = scan.getTable().getInvertedIndexFileStorageFormat()
+ == TInvertedIndexFileStorageFormat.SNII;
Map<String, String> properties = selectedIndex.getProperties();
- if (properties == null
- || !properties.containsKey(
-
InvertedIndexProperties.INVERTED_INDEX_ANALYZER_NAME_KEY)) {
+ String analyzerName = properties == null ? null : properties.get(
+ InvertedIndexProperties.INVERTED_INDEX_ANALYZER_NAME_KEY);
+ if (analyzerName == null) {
+ if (isSnii) {
Review Comment:
[P1] Check only predicates that contribute to score
This rejects every SNII index encountered in the filter, even when that
predicate is only a boolean filter. For example:
```text
TopN(order by s)
Project(score() AS s)
Filter(search('body:ANY(alpha) AND tag:pre*'))
Scan(SNII; body=CommonGrams, tag=plain index)
```
RewriteSearchToSlots binds both fields, so this branch rejects the
analyzerless tag index. BE scoring deliberately ignores that leaf:
SearchPredicateCollector returns unless the type is TERM, EXACT, PHRASE, MATCH,
ANY, or ALL; PREFIX, WILDCARD, REGEXP, RANGE, and LIST remain ordinary filters.
The direct-MATCH loop has the same problem for a mixed `body MATCH_ANY 'alpha'
AND tag MATCH_REGEXP 'pre.*'` plan: MatchPredicateCollector emits no scoring
info for MATCH_REGEXP or MATCH_PHRASE_EDGE. In both cases the score is
well-defined from body. Please mirror the BE contributor classification for
both MATCH and SEARCH and add mixed scoring/non-scoring predicate tests.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CheckScoreUsage.java:
##########
@@ -133,31 +133,42 @@ private static void
checkSelectedIndexPolicyAdmission(Index selectedIndex,
if (selectedIndex == null) {
return;
}
+ boolean isSnii = scan.getTable().getInvertedIndexFileStorageFormat()
+ == TInvertedIndexFileStorageFormat.SNII;
Map<String, String> properties = selectedIndex.getProperties();
- if (properties == null
- || !properties.containsKey(
-
InvertedIndexProperties.INVERTED_INDEX_ANALYZER_NAME_KEY)) {
+ String analyzerName = properties == null ? null : properties.get(
+ InvertedIndexProperties.INVERTED_INDEX_ANALYZER_NAME_KEY);
+ if (analyzerName == null) {
+ if (isSnii) {
+ throw nonScoringSniiIndex(selectedIndex);
+ }
return;
}
- String analyzerName = properties.get(
- InvertedIndexProperties.INVERTED_INDEX_ANALYZER_NAME_KEY);
boolean usesCommonGrams;
try {
usesCommonGrams =
indexPolicyMgr.validateAnalyzerUsesCommonGrams(analyzerName);
} catch (DdlException e) {
throw new AnalysisException("score() cannot use inverted index '"
+ selectedIndex.getIndexName() + "': " + e.getMessage(),
e);
}
- if (usesCommonGrams
- && scan.getTable().getInvertedIndexFileStorageFormat()
- != TInvertedIndexFileStorageFormat.SNII) {
+ if (isSnii && !usesCommonGrams) {
+ throw nonScoringSniiIndex(selectedIndex);
+ }
+ if (usesCommonGrams && !isSnii) {
throw new AnalysisException("score() cannot use CommonGrams
analyzer '"
+ analyzerName + "' on inverted index '" +
selectedIndex.getIndexName()
+ "': CommonGrams scoring is supported only by SNII");
}
}
+ private static AnalysisException nonScoringSniiIndex(Index selectedIndex) {
+ return new AnalysisException("score() cannot use SNII inverted index '"
+ + selectedIndex.getIndexName()
+ + "': the index does not persist scoring data; SNII scoring
requires"
+ + " an index created with a CommonGrams analyzer");
Review Comment:
[P1] Gate on persisted scoring capability, not analyzer type
This treats the current analyzer graph as proof that the index actually
contains scoring data:
```text
TopN(order by s)
Project(score() AS s)
Filter(body MATCH_PHRASE 'alpha beta')
Scan(SNII, CommonGrams policy; a visible segment was written with the
build switch off)
```
validateAnalyzerUsesCommonGrams returns true, but SniiIndexColumnWriter
snapshots mutable enable_common_grams_index_build and writes the docs/positions
tier without norms or semantic metadata when it is false. Existing cases in
test_common_grams_snii.groovy:885-930 and 932-978 construct all-missing and
mixed CommonGrams segments and fail later with `SNII semantic scoring metadata
is missing`; legacy or replica-divergent segments have the same shape.
CommonGrams is therefore necessary, not sufficient. Please gate on a durable
capability covering every visible segment/replica, or retain only unconditional
FE rejections and leave persisted-capability admission authoritative in BE. Add
missing/mixed-segment coverage instead of only mocking the policy boolean.
--
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]
