airborne12 commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4091211725
##########
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:
You are right, and my previous reply on this was wrong: I said that putting
the check in `read_from_index()` meant no caller could route around it. Direct
SEARCH does exactly that - `FieldReaderResolver::resolve()` calls
`select_best_reader()` itself (`variant_inverted_index_search.cpp:197,200`) and
then queries the reader or builds the phrase query without ever entering
`read_from_index()`. Fixed in 4ce410ca359.
The check now lives inside `select_best_reader()` itself, in the overload
that takes the query type, right after the candidate is chosen. That overload
is the one point both entry paths share, so MATCH through `read_from_index()`
and SEARCH through the resolver are covered by the same lines, and the copy in
`read_from_index()` is gone.
Test: `InvertedIndexIteratorTest.PhraseSupportIsCheckedOnTheSelectedReader`
now asserts that selecting the index without positions for a phrase query fails
with `INDEX_INVALID_PARAMETERS` at selection time, in both index orders, while
a non-positional query on the same index still selects it. Reverting the change
makes it fail (`Actual: true, Expected: false` on the selection result), and
since it drives `select_best_reader()` directly it exercises the path SEARCH
takes rather than only the MATCH one.
##########
be/src/storage/index/inverted/analyzer/custom_analyzer.cpp:
##########
@@ -70,12 +70,17 @@ ReaderPtr CustomAnalyzer::init_reader(ReaderPtr reader) {
}
TokenStreamComponentsPtr CustomAnalyzer::create_components() {
- auto tk = _tokenizer->create();
- TokenStreamPtr ts = tk;
- for (const auto& filter : _token_filters) {
- ts = filter->create(ts);
+ try {
+ auto tk = _tokenizer->create();
+ TokenStreamPtr ts = tk;
+ for (const auto& filter : _token_filters) {
+ ts = filter->create(ts);
+ }
+ return std::make_shared<TokenStreamComponents>(tk, ts);
+ } catch (const CLuceneError& e) {
+ throw Exception(ErrorCode::INVERTED_INDEX_ANALYZER_ERROR,
Review Comment:
Confirmed and fixed in 4ce410ca359.
The four tokenizing sites in `build_leaf_query()` - the SNII TERM
minimum-should-match pre-analysis and the CLucene TERM, PHRASE and ANY/ALL
builders - call `get_analyse_result()` with no catch anywhere in the file, and
`VSearchExpr` enters `evaluate_inverted_index_with_search_param()` directly
(`vsearch.cpp:257`), so IFunction's `catch (const Exception&)` around
`execute()` never sees this path.
Rather than wrapping each call, the fix restores the missing boundary once:
the overload `VSearchExpr` calls now runs the body through a `try` that
converts `Exception` with `e.to_status()`, the same way `IFunction::execute()`
does, plus a `CLuceneError` clause since CLucene tokenizers raise that from
inside a token stream. Anything that throws below that point, at construction
or on first use, becomes a Status, so the per-call wrapping does not have to be
kept complete by hand.
On the test, one honest note. I first tried to reproduce the literal case -
provider built, first token stream throws - and could not do it
deterministically: the ik and icu dictionaries are process-wide once-guarded
singletons (`Dictionary::initial()` is a `std::call_once`, the icu config keeps
a static `initialized_` flag), so whichever test touches them first in the
shared binary decides their state, and the tokenizers that validate eagerly
(ngram) fail at construction, which the provider-construction helpers already
convert to a Status before any tokenization. So
`FunctionSearchTest.SearchConvertsExceptionInsideSearchToStatus` drives the
boundary with a bound SNII reader whose query throws a `doris::Exception`: the
resolver binds it normally, the search then raises inside the guarded body, and
the test asserts the call returns a Status carrying that message instead of
throwing. With the boundary reverted, the same test fails because the exception
escapes the call.
--
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]
