airborne12 commented on code in PR #69790:
URL: https://github.com/apache/doris/pull/69790#discussion_r4412691841
##########
be/src/storage/index/inverted/reader/predicate_collector.cpp:
##########
@@ -285,6 +285,24 @@ void preserve_selected_index_metadata(const
ScoringIndexCandidates& candidates,
using namespace scoring_collection_detail;
+namespace {
+
+// The terms a MATCH scores, the ones its reader looks up. A keyword index
stores the whole value
+// as one term, case and spaces included: tag MATCH 'Hello World' scores the
single term
+// "Hello World", as SEARCH('tag:"Hello World"') does. Any other index cuts
the value with the
+// query's analyzer.
+Result<std::vector<TermInfo>> match_scoring_terms(const std::string& value,
+ const TabletIndex&
index_meta,
+ const
InvertedIndexAnalyzerCtx& analyzer_ctx) {
+ if
(!inverted_index::InvertedIndexAnalyzer::should_analyzer(index_meta.properties()))
{
+ return std::vector<TermInfo> {TermInfo(value)};
Review Comment:
Fixed in db7d9a86dc. Confirmed, and the effect was worse than a wrong IDF:
the scorer looked up `BP`, found no statistics for it, and the query failed
with `Not such term`. For a keyword MATCH_PHRASE the collector now strips the
slop with the same `take_slop` that `lower_match` (and the no-index path in
`match.cpp`) uses, so it collects the term the reader looks up. Covered by
`CollectionStatisticsTest.KeywordPhraseCollectsTermWithoutSlop` (`~N` and
`~N+`; MATCH_ANY keeps the whole value) and the `match_phrase_slop` /
`match_phrase_ordered_slop` cases in `test_match_keyword_score` (V2/V3/SNII),
which score the same as `tag MATCH_PHRASE 'BP'`.
##########
be/src/storage/index/index_reader_helper.cpp:
##########
@@ -24,12 +24,16 @@ namespace {
bool can_score_match(const TabletIndex* index_meta) {
const auto& properties = index_meta->properties();
- // parser=none can inherit support_phrase=true from FE defaults. MATCH
statistics skip
- // these keyword indexes, so their filters must not request a scoring
context either.
- return inverted_index::InvertedIndexAnalyzer::should_analyzer(properties)
&&
- (get_parser_phrase_support_string_from_properties(properties) ==
- INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES ||
- IndexReaderHelper::is_normalizer_only(properties));
+ // A keyword index keeps each value as one term, without positions or
norms, and its postings
+ // score with unit frequency and length whatever support_phrase says, as
an Elasticsearch
+ // keyword field does: over tags (x, x, y, z), tag MATCH 'x' scores idf(x)
= ln(2) for both
+ // matching rows, the same as SEARCH('tag:x'). MATCH statistics collect
these terms too.
+ if (!inverted_index::InvertedIndexAnalyzer::should_analyzer(properties)) {
+ return true;
+ }
Review Comment:
Fixed in db7d9a86dc. Confirmed: `find_slot_ref` looks through the cast to
`n`, the selector picks its BKD index for an INT field, and a BKD index has no
analyzer, so the keyword rule admitted it. Only a text index is ever read with
a score (`TextIndexReader` is the only reader-side caller of
`is_need_similarity_score`), so `MatchPredicateCollector` now collects nothing
for a MATCH on a non-string leaf. The SEARCH collector returns an error for a
numeric leaf instead (`SearchScoringRejectsNumericBkdLeaf`), but here the
predicate is only a scalar filter, so returning an error would fail a query
that used to run. Covered by
`CollectionStatisticsTest.MatchOnNumericColumnCollectsNothing` and the
`analyzed_and_numeric_cast` case in `test_match_keyword_score` (`body MATCH
'apple' AND CAST(n AS STRING) MATCH '1'`, V2/V3/SNII), where only the body leaf
scores.
##########
be/src/storage/index/inverted/reader/predicate_collector.cpp:
##########
@@ -365,15 +383,14 @@ Status MatchPredicateCollector::collect(RuntimeState*
state, const TabletSchemaS
const auto* index_meta = DORIS_TRY(
select_index_meta(candidates.index_metas, candidates.field_type,
query_type,
analyzer_ctx->analyzer_key,
analyzer_ctx->legacy_analyzer_key));
- if
(!inverted_index::InvertedIndexAnalyzer::should_analyzer(index_meta->properties())
||
- !IndexReaderHelper::is_need_similarity_score(expr->op(), index_meta)) {
+ if (!IndexReaderHelper::is_need_similarity_score(expr->op(), index_meta)) {
Review Comment:
Confirmed, but this is out of scope for this PR. The restriction is
`validate_same_physical_index`, which exists on master already: statistics are
keyed per field (column unique id + suffix path), and scoring two different
inverted indexes of one column in one query has always been rejected. Two
analyzed indexes on one column, a path this PR does not change, fail with the
same error. I checked it locally: with `parser=english` and `parser=standard`
indexes on `name`, `WHERE name MATCH 'apple' USING ANALYZER english AND name
MATCH 'apple' USING ANALYZER standard ORDER BY score() DESC LIMIT 10` returns
"Scoring predicates for one field select different inverted indexes". This PR
makes keyword indexes score, so a keyword + analyzed pair on one column now
falls under that same existing rule. Before, the keyword leaf did not
participate in scoring at all, which is why that pair used to run.
Lifting the restriction means keying the collected statistics and the scorer
lookup by physical index across the collector, `CollectionStatistics` and the
V2/V3/SNII readers. That is a separate change, and it applies equally to the
analyzed + analyzed case, so I'd rather do it in its own PR than grow this fix.
The query needs two indexes on the same column and both of them used in one
`score()` query with different `USING ANALYZER` clauses, so it is rare in
practice.
--
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]
