airborne12 commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4092656835
##########
be/src/storage/index/inverted/inverted_index_iterator.cpp:
##########
@@ -69,12 +70,21 @@ Status InvertedIndexIterator::read_from_index(const
IndexParam& param) {
// The execution context carries reader selection separately from analyzer
execution.
const std::string& analyzer_key =
(i_param->analyzer_ctx != nullptr) ?
i_param->analyzer_ctx->analyzer_key : "";
- auto reader =
- DORIS_TRY(select_best_reader(i_param->column_type,
i_param->query_type, analyzer_key));
+ const std::string& legacy_analyzer_key =
+ (i_param->analyzer_ctx != nullptr) ?
i_param->analyzer_ctx->legacy_analyzer_key : "";
+ auto reader = DORIS_TRY(select_best_reader(i_param->column_type,
i_param->query_type,
+ analyzer_key,
legacy_analyzer_key));
if (UNLIKELY(reader == nullptr)) {
return Status::Error<ErrorCode::INVERTED_INDEX_CLUCENE_ERROR>(
"inverted index reader is null");
}
+ // Check the reader that runs the query, not the first candidate of its
type, because the
+ // analyzer decides which index is selected and the two can disagree on
support_phrase.
+ if (is_phrase_query(i_param->query_type) &&
Review Comment:
Follow-up: the change in 4ce410ca359 was too broad and CI caught it, so I
have taken it back in 4a30b86113b.
What broke: putting the check inside `select_best_reader()` made it run for
every selected reader and for every caller. The old preflight only ever looked
at `get_reader(FULLTEXT)`, so it never applied to an untokenized index, which
does not declare `support_phrase` and answers `MATCH_PHRASE` by matching the
whole value as a single term. P0 failed
`inverted_index_p0.test_index_complex_match` (`clientip MATCH_PHRASE
'247.37.0.0'` on a plain `USING INVERTED` index) and NonConcurrent failed
`inverted_index_p0.test_omit_norms` (`request MATCH_PHRASE_PREFIX ...` on an
index without a parser), both with E-6000.
Direct SEARCH is the other half. It has never enforced `support_phrase`, and
its regression suites rely on that: `search.test_search_function` runs
`search('content:"machine learning"')` and `search.test_search_dsl_syntax` runs
`NOT search('content:"success message"')` against `idx_content ...
PROPERTIES("parser" = "english")` with no `support_phrase`, and their `.out`
files expect 1 and 19 rows. With the shared check those suites and
`test_search_multi_analyzer_lucene` failed with E-6000. Whether SEARCH should
start rejecting phrase clauses on indexes without positions is a behaviour
change with its own regression contract, so it is out of scope for this PR;
SEARCH keeps its previous behaviour.
Now: the check sits in `read_from_index()`, i.e. the MATCH path only, and it
runs only when the selected reader is a FULLTEXT reader. It still checks the
reader the analyzer selected rather than the first full-text candidate, which
is what the original finding in this thread was about.
Tests: `InvertedIndexIteratorTest.PhraseSupportIsCheckedOnTheSelectedReader`
now drives `read_from_index()` and asserts the positional index is queried
while the index without positions is rejected, in both index orders;
`PhraseQueriesStillRunOnAnUntokenizedIndex` asserts MATCH_PHRASE /
MATCH_PHRASE_PREFIX / MATCH_PHRASE_EDGE still query a STRING_TYPE reader that
has no `support_phrase`. Forcing the check off makes the first fail; restoring
the previous head makes the second fail.
--
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]
