github-actions[bot] commented on code in PR #67180:
URL: https://github.com/apache/doris/pull/67180#discussion_r3864368344
##########
be/src/storage/index/inverted/query/phrase_query.cpp:
##########
@@ -53,6 +53,14 @@ void PhraseQuery::add(const InvertedIndexQueryInfo&
query_info) {
init_ordered_sloppy_phrase_matcher(query_info, is_similarity);
}
+ // Two-phase evaluation with a pushed-down candidate set: the candidate
+ // bitmap joins the leapfrog intersection (restricting doc-list walking and
+ // position verification to candidates) but never a matcher's postings, so
+ // phrase semantics stay with the real term iterators.
+ if (_context->candidate_rows != nullptr) {
+
_iterators.emplace_back(std::make_shared<RoaringDocIdIterator>(_context->candidate_rows));
Review Comment:
[P1] Preserve a real postings norm when the candidate leads. The sort makes
a selective RoaringDocIdIterator become `_lead1`, but `search_by_skiplist()`
later obtains `Norm` from `_lead1` and this adapter returns the constant 1. For
scoring phrase and multi-term phrase-prefix queries, documents of different
lengths therefore get wrong BM25 values, which can change score filters and
top-k results. Please keep a real term iterator as the norm source (or do not
use the candidate as a scoring lead) and add score-equivalence coverage.
##########
be/src/common/config.cpp:
##########
@@ -1319,6 +1319,7 @@ DEFINE_Bool(enable_inverted_index_cache_check_timestamp,
"true");
DEFINE_mBool(enable_inverted_index_correct_term_write, "true");
DEFINE_Int32(inverted_index_fd_number_limit_percent, "20"); // 20%
DEFINE_Int32(inverted_index_query_cache_shards, "256");
+DEFINE_mDouble(inverted_index_candidate_pushdown_ratio, "0.1");
Review Comment:
[P2] Make this default both safe and consistent with the workload used to
justify it. Doris' generic double parser accepts values such as `inf` (and very
large finite values), while SegmentIterator multiplies the value and casts the
result to `uint64_t`; a non-finite or out-of-range floating-to-integer
conversion is undefined behavior. A post-assignment validator alone is
insufficient because the runtime update path can transiently publish the
rejected value before rollback. Separately, the PR reports that short-key
pruning leaves 22.4% of each segment and claims an approximately 4.5x phrase
improvement with only that pruning, but the new 0.1 default requires less than
10%, so the motivating workload never publishes `candidate_rows` and runs the
old full-postings path. Please prevalidate a finite value in the intended
domain, make the arithmetic/publication intrinsically safe, calibrate the
default with crossover evidence (or document the required override and correct
the claim),
and cover startup/runtime and production-boundary behavior.
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -834,8 +834,28 @@ Status
SegmentIterator::_get_row_ranges_by_column_conditions() {
if (_index_query_context != nullptr) {
_index_query_context->count_on_index_fastpath =
_count_on_index_fastpath_safe();
_index_query_context->count_on_index_fastpath_hit = false;
+ // Candidate-pushdown handshake: while index conditions are
+ // evaluated, expose the current candidate bitmap so index
+ // queries can restrict themselves to it (two-phase
+ // evaluation). Only engaged when the candidate set is small
+ // enough for the restriction to pay off; results produced
+ // under it are partial and skip the query cache. _row_bitmap
+ // only shrinks during the applies below, so restricting to
+ // its current state stays correct for every later conjunct.
+ double candidate_ratio =
config::inverted_index_candidate_pushdown_ratio;
Review Comment:
[P2] Re-evaluate the threshold after earlier index conjuncts have been
applied. This gate is sampled before `_apply_inverted_index()`, so if the entry
bitmap is 20% (above the default 0.1 threshold) and an indexed equality then
reduces it to 0.1%, `candidate_rows` remains null for the later phrase
expression and the phrase still walks the full segment. That misses the
selective-companion path this optimization is meant to accelerate. Please apply
and refresh the handshake at safe conjunct boundaries and add a
SegmentIterator-level threshold-crossing test.
--
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]
