airborne12 commented on PR #66872:
URL: https://github.com/apache/doris/pull/66872#issuecomment-5412877147
Closing this. Its premise no longer holds.
This PR taught the FE that an SNII index can only be ranked when it was
created with a CommonGrams analyzer, and rejected everything else at analysis
time. That was a faithful description of what the BE did, but the BE behaviour
was itself the defect: CommonGrams is a phrase-query performance optimization,
and scoring had been coupled to it only because the semantic collection
statistics it needs were introduced inside the CommonGrams segment metadata.
V1/V2/V3 rank an ordinary
`PROPERTIES("parser"="english","support_phrase"="true")` index perfectly well
(`test_bm25_score.groovy`), so SNII was the outlier.
#67134 decouples them: any analyzed SNII index with positions now reaches
the scoring tier, persists norms, and is rankable — including ARRAY columns,
which CommonGrams rejects outright and which therefore could never be ranked on
SNII at all. With that landed, the rule this PR adds would reject queries that
work.
**What happens to the three review findings**, since two of them are worth
keeping and one is not:
- **[P1] Check only predicates that contribute to score** — real, and
confirmed: `SearchPredicateCollector::is_score_query_type` admits only
TERM/EXACT/PHRASE/MATCH/ANY/ALL, and
`IndexReaderHelper::is_need_similarity_score` only
MATCH_ANY/ALL/PHRASE/PHRASE_PREFIX, so a PREFIX leaf or a MATCH_REGEXP
predicate contributes nothing and is silently skipped by the BE. I reproduced
the over-rejection at FE unit level for both the SEARCH and the direct-MATCH
shape. It becomes moot here only because the whole rule goes away; if an
FE-side admission check is ever reintroduced it must mirror that classification.
- **[P1] Gate on persisted scoring capability, not analyzer type** —
correct, and the reason this PR was the wrong layer.
`test_common_grams_snii.groovy` already constructs a CommonGrams-analyzer table
whose segments were written with `enable_common_grams_index_build=false` and
asserts the BE-side failure, so the analyzer graph never was proof that scoring
data exists. #67134 makes the persisted capability (scoring tier + positions +
norms) the single predicate, in the BE, where it belongs.
- **[P1] Resolve SEARCH admission with the runtime query type** — this one
**outlives the PR** and should not be lost. On a column carrying both an
untokenized index and an analyzed one, `OlapTable.getInvertedIndex(column,
subPath, null)` returns the first index for which `isAnalyzedInvertedIndex()`
is true, regardless of declaration order, while the BE routes an EXACT clause
to `InvertedIndexReaderType::STRING_TYPE` (`inverted_index_selector.cpp`,
`is_equal_query` branch). FE and BE can therefore pick different physical
indexes for the same leaf. `test_search_exact_multi_index.groovy` shows the
multi-index-per-column shape is a supported, tested feature. That divergence is
independent of scoring and of this PR; it deserves its own issue rather than
being carried by a rule that is being deleted.
Thanks for the review — it was right on all three counts, and the third is
the one still worth chasing.
--
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]
