From af66197c995e5a3886d56c1e4cf6fe31f4ad7e42 Mon Sep 17 00:00:00 2001 From: airborne12 Date: Wed, 26 Aug 2026 22:49:40 +0800 Subject: [PATCH 1/5] [improvement](inverted index) Push the candidate row bitmap down into phrase queries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What problem does this PR solve? Problem Summary: The cost of a phrase-family inverted index query (MATCH_PHRASE / MATCH_PHRASE_PREFIX multi-term path) is proportional to the whole segment's postings and positions, regardless of how small the surviving candidate set already is. On a production log table a 9-minute time window left only 22.4% of each segment's rows after short-key pruning, yet every phrase conjunct still walked the full segment: per-query profile showed 2,657 segments, 11,913s of InvertedIndexSearcherSearchExecTime (99.7% of scan cost) and 125 GiB of index reads for a query whose final result was 0 rows. A microbenchmark against the same code path calibrates the cost model to ~0.9µs per co-occurrence candidate, so evaluation cost tracks candidates, not results. Fix: expose the scan's current candidate row bitmap to index queries through IndexQueryContext (the same SegmentIterator -> reader handshake channel the count-on-index fast path already uses): - `IndexQueryContext::candidate_rows`: set by SegmentIterator around the index-apply phase when the candidate bitmap is smaller than `num_rows * inverted_index_candidate_pushdown_ratio` (new BE config, default 0.1, <= 0 disables), reset on every exit path via the existing DEFER. `_row_bitmap` only shrinks during the applies, so restricting to its current state stays correct for every later conjunct. - `RoaringDocIdIterator`: a read-only DISI adapter over the candidate bitmap. PhraseQuery joins it into the leapfrog intersection (its doc_freq() is the cardinality, so a small candidate naturally becomes the lead), while matchers keep only real term iterators -- a classic two-phase iterator: candidates drive the approximation, terms keep the position semantics. The single-term path is unchanged (no restriction, same semantics). - Query cache: a bitmap produced under a non-null candidate is partial and is never inserted into the query cache. Cache lookups stay enabled -- a cached full-segment bitmap intersected later is still correct and cheaper. While a candidate is engaged this also skips caching for non-phrase fulltext queries (conservative but correct; the restriction only engages below the ratio threshold where such caching has little value). Expected effect on the reproduced workload: with only short-key pruning the phrase work drops to the in-window fraction of each segment (~4.5x on the profiled table, more for smaller windows); combined with selective companion conjuncts the candidate set collapses further and so does the phrase cost. Results are unchanged -- verified by an equivalence test where a full-coverage candidate reproduces the unrestricted result. ### Release note Inverted index: phrase queries now restrict doc-list intersection and position verification to the scan's surviving candidate rows when the candidate set is small (BE config inverted_index_candidate_pushdown_ratio, default 0.1). ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason - Behavior changed: - [ ] No. - [x] Yes. - Does this need documentation? - [x] No. - [ ] Yes. --- be/src/common/config.cpp | 1 + be/src/common/config.h | 5 + be/src/storage/index/index_query_context.h | 11 ++ .../index/inverted/inverted_index_reader.cpp | 6 +- .../index/inverted/query/phrase_query.cpp | 8 ++ .../index/inverted/util/docid_set_iterator.h | 3 +- .../inverted/util/roaring_docid_iterator.h | 88 ++++++++++++ be/src/storage/segment/segment_iterator.cpp | 22 ++- .../inverted/query/phrase_query_test.cpp | 135 ++++++++++++++++++ 9 files changed, 276 insertions(+), 3 deletions(-) create mode 100644 be/src/storage/index/inverted/util/roaring_docid_iterator.h diff --git a/be/src/common/config.cpp b/be/src/common/config.cpp index 03266539c4dc8e..0089e9b6725d5e 100644 --- a/be/src/common/config.cpp +++ b/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"); // inverted index match bitmap cache size DEFINE_String(inverted_index_query_cache_limit, "10%"); diff --git a/be/src/common/config.h b/be/src/common/config.h index 436a1878ef424a..2224aeb8229e0e 100644 --- a/be/src/common/config.h +++ b/be/src/common/config.h @@ -1372,6 +1372,11 @@ DECLARE_Bool(enable_inverted_index_cache_check_timestamp); DECLARE_mBool(enable_inverted_index_correct_term_write); DECLARE_Int32(inverted_index_fd_number_limit_percent); // 50% DECLARE_Int32(inverted_index_query_cache_shards); +// When the candidate row bitmap of a segment scan is smaller than +// num_rows * this ratio, it is pushed down into inverted index queries so +// doc-list intersection and verification run only over the candidates +// (see IndexQueryContext::candidate_rows). <= 0 disables the pushdown. +DECLARE_mDouble(inverted_index_candidate_pushdown_ratio); // inverted index match bitmap cache size DECLARE_String(inverted_index_query_cache_limit); diff --git a/be/src/storage/index/index_query_context.h b/be/src/storage/index/index_query_context.h index 7cdb609a234654..3d7a7ed5cbea56 100644 --- a/be/src/storage/index/index_query_context.h +++ b/be/src/storage/index/index_query_context.h @@ -48,6 +48,17 @@ struct IndexQueryContext { // under a key a row-accurate query could hit. bool count_on_index_fastpath = false; + // Candidate-pushdown handshake. Set by SegmentIterator ONLY while it + // evaluates pushed-down conjuncts and the current candidate row bitmap is + // small enough (config::inverted_index_candidate_pushdown_ratio), reset + // right after. When set, an index query MAY restrict doc-list intersection + // and verification to this candidate set (PhraseQuery joins it into the + // leapfrog). A bitmap produced under a non-null candidate is PARTIAL and + // must never be inserted into the query cache; cache lookups stay valid + // (a cached full-segment bitmap intersected later is still correct). The + // pointee is owned by the caller and outlives the evaluation. + const roaring::Roaring* candidate_rows = nullptr; + // ---- Reply direction: fields a READER writes and the CALLER reads back ---- // // A caller that hands a reader a COPY of this context rather than the context itself must diff --git a/be/src/storage/index/inverted/inverted_index_reader.cpp b/be/src/storage/index/inverted/inverted_index_reader.cpp index a119992538b5e5..01b48fc1f95c79 100644 --- a/be/src/storage/index/inverted/inverted_index_reader.cpp +++ b/be/src/storage/index/inverted/inverted_index_reader.cpp @@ -477,7 +477,11 @@ Status FullTextIndexReader::query(const IndexQueryContextPtr& context, RETURN_IF_ERROR(match_index_search(context, query_type, query_info, *searcher_ptr, term_match_bitmap)); term_match_bitmap->runOptimize(); - cache->insert(cache_key, term_match_bitmap, &cache_handler); + // A bitmap produced under a candidate restriction is partial and + // must never be cached as the full-segment result. + if (context->candidate_rows == nullptr) { + cache->insert(cache_key, term_match_bitmap, &cache_handler); + } bit_map = term_match_bitmap; } return Status::OK(); diff --git a/be/src/storage/index/inverted/query/phrase_query.cpp b/be/src/storage/index/inverted/query/phrase_query.cpp index 6b0dc052091a23..7891be64945aa1 100644 --- a/be/src/storage/index/inverted/query/phrase_query.cpp +++ b/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(_context->candidate_rows)); + } + std::sort(_iterators.begin(), _iterators.end(), [](const DISI& a, const DISI& b) { int64_t freq1 = visit_node(a, DocFreq {}); int64_t freq2 = visit_node(b, DocFreq {}); diff --git a/be/src/storage/index/inverted/util/docid_set_iterator.h b/be/src/storage/index/inverted/util/docid_set_iterator.h index 00ffb1e8c750ed..73277ac49b50a1 100644 --- a/be/src/storage/index/inverted/util/docid_set_iterator.h +++ b/be/src/storage/index/inverted/util/docid_set_iterator.h @@ -21,11 +21,12 @@ #include "common/exception.h" #include "storage/index/inverted/util/mock_iterator.h" +#include "storage/index/inverted/util/roaring_docid_iterator.h" #include "storage/index/inverted/util/union_term_iterator.h" namespace doris::segment_v2 { -using DISI = std::variant; +using DISI = std::variant; template auto visit_node(DISIType&& disi, Func&& func, Args&&... args) { diff --git a/be/src/storage/index/inverted/util/roaring_docid_iterator.h b/be/src/storage/index/inverted/util/roaring_docid_iterator.h new file mode 100644 index 00000000000000..d7d22f34d7e66a --- /dev/null +++ b/be/src/storage/index/inverted/util/roaring_docid_iterator.h @@ -0,0 +1,88 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#include +#include +#include + +#include "roaring/roaring.hh" + +namespace doris::segment_v2 { + +// Read-only DISI adapter over a candidate row bitmap +// (IndexQueryContext::candidate_rows). Joining the leapfrog intersection of +// PhraseQuery, it restricts doc-list intersection and position verification to +// the candidate set (two-phase evaluation: this iterator drives the +// approximation, real term iterators keep the position semantics). It never +// joins a matcher's postings, so freq()/next_position()/norm() only satisfy +// the DISI interface with neutral values. +// +// The underlying bitmap is NOT owned and must outlive the iterator; leapfrog +// only moves forward, so advance() targets are monotonically non-decreasing. +class RoaringDocIdIterator { +public: + explicit RoaringDocIdIterator(const roaring::Roaring* rows) + : _rows(rows), _iter(rows->begin()) {} + + // DISI convention: an iterator starts positioned BEFORE its first doc + // (search_by_skiplist opens with a NextDoc on the lead), so doc_id() is -1 + // until the first next_doc()/advance() moves onto a real position. + int32_t doc_id() const { + if (!_started) { + return -1; + } + return _iter == _rows->end() ? INT_MAX : static_cast(*_iter); + } + + int32_t freq() const { return 1; } + + int32_t next_doc() { + if (!_started) { + _started = true; + } else if (_iter != _rows->end()) { + ++_iter; + } + return doc_id(); + } + + int32_t advance(int32_t target) { + _started = true; + if (target > 0) { + _iter.equalorlarger(static_cast(target)); + } + return doc_id(); + } + + int32_t doc_freq() const { + uint64_t cardinality = _rows->cardinality(); + return cardinality > INT_MAX ? INT_MAX : static_cast(cardinality); + } + + int32_t next_position() { return 0; } + + int32_t norm() const { return 1; } + +private: + const roaring::Roaring* _rows; + roaring::Roaring::const_iterator _iter; + bool _started = false; +}; +using RoaringDocIdIterPtr = std::shared_ptr; + +} // namespace doris::segment_v2 diff --git a/be/src/storage/segment/segment_iterator.cpp b/be/src/storage/segment/segment_iterator.cpp index 08ec84d700fe53..4001da5889027c 100644 --- a/be/src/storage/segment/segment_iterator.cpp +++ b/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; + if (candidate_ratio > 0 && + _row_bitmap.cardinality() < + static_cast(static_cast(num_rows()) * + candidate_ratio)) { + _index_query_context->candidate_rows = &_row_bitmap; + } } - DEFER({ _capture_count_fastpath_hit(); }); + DEFER({ + _capture_count_fastpath_hit(); + if (_index_query_context != nullptr) { + _index_query_context->candidate_rows = nullptr; + } + }); // Only apply column-level inverted index if we have iterators if (has_index_in_iterators()) { RETURN_IF_ERROR(_apply_inverted_index()); diff --git a/be/test/storage/index/inverted/query/phrase_query_test.cpp b/be/test/storage/index/inverted/query/phrase_query_test.cpp index 40f7f59349dbb5..cb61ee9a9f22d9 100644 --- a/be/test/storage/index/inverted/query/phrase_query_test.cpp +++ b/be/test/storage/index/inverted/query/phrase_query_test.cpp @@ -665,4 +665,139 @@ TEST_F(PhraseQueryTest, test_parser_slop) { } } +// With IndexQueryContext::candidate_rows set, the phrase leapfrog must be +// restricted to the candidate set: docs outside it are neither intersected +// nor position-verified, and never appear in the result. +TEST_F(PhraseQueryTest, candidate_rows_restrict_phrase_search) { + std::string_view rowset_id = "test_candidate_restrict"; + int seg_id = 0; + + std::vector values = { + Slice("big red apple"), // doc 0 - matches "big red" + Slice("small red apple"), // doc 1 - no match + Slice("big blue apple"), // doc 2 - no match + Slice("red big apple"), // doc 3 - no match (wrong order) + Slice("big red orange"), // doc 4 - matches "big red" + Slice("very big red car") // doc 5 - matches "big red" + }; + + TabletIndex idx_meta; + auto index_meta_pb = std::make_unique(); + index_meta_pb->set_index_type(IndexType::INVERTED); + index_meta_pb->set_index_id(1); + index_meta_pb->set_index_name("test_candidate_restrict"); + index_meta_pb->clear_col_unique_id(); + index_meta_pb->add_col_unique_id(1); + index_meta_pb->mutable_properties()->insert({"parser", "english"}); + index_meta_pb->mutable_properties()->insert({"lower_case", "true"}); + index_meta_pb->mutable_properties()->insert({"support_phrase", "true"}); + idx_meta.init_from_pb(*index_meta_pb.get()); + + std::string index_path_prefix; + prepare_fulltext_index(rowset_id, seg_id, values, &idx_meta, &index_path_prefix); + auto searcher = create_searcher(index_path_prefix, idx_meta); + ASSERT_NE(searcher, nullptr); + + RuntimeState runtime_state; + io::IOContext io_ctx; + + InvertedIndexQueryInfo query_info; + query_info.field_name = L"1"; + query_info.term_infos.emplace_back("big", 0); + query_info.term_infos.emplace_back("red", 1); + query_info.slop = 0; + + // Calibration: without a candidate the phrase matches docs {0, 4, 5}. + { + IndexQueryContextPtr context = std::make_shared(); + context->io_ctx = &io_ctx; + context->runtime_state = &runtime_state; + PhraseQuery query(searcher, context); + query.add(query_info); + roaring::Roaring baseline; + query.search(baseline); + ASSERT_EQ(baseline.cardinality(), 3); + ASSERT_TRUE(baseline.contains(0)); + ASSERT_TRUE(baseline.contains(4)); + ASSERT_TRUE(baseline.contains(5)); + } + + // Candidate {0, 5}: doc 4 matches the phrase but is outside the candidate + // set, so it must not appear. + { + roaring::Roaring candidate; + candidate.add(0); + candidate.add(5); + + IndexQueryContextPtr context = std::make_shared(); + context->io_ctx = &io_ctx; + context->runtime_state = &runtime_state; + context->candidate_rows = &candidate; + PhraseQuery query(searcher, context); + query.add(query_info); + roaring::Roaring result; + query.search(result); + EXPECT_EQ(result.cardinality(), 2); + EXPECT_TRUE(result.contains(0)); + EXPECT_TRUE(result.contains(5)); + EXPECT_FALSE(result.contains(4)); + } +} + +// A candidate covering every row must not change the result (equivalence with +// the full-segment evaluation). +TEST_F(PhraseQueryTest, candidate_rows_full_cover_keeps_results) { + std::string_view rowset_id = "test_candidate_full_cover"; + int seg_id = 0; + + std::vector values = { + Slice("big red apple"), // doc 0 - matches + Slice("small red apple"), // doc 1 + Slice("big blue apple"), // doc 2 + Slice("big red orange") // doc 3 - matches + }; + + TabletIndex idx_meta; + auto index_meta_pb = std::make_unique(); + index_meta_pb->set_index_type(IndexType::INVERTED); + index_meta_pb->set_index_id(1); + index_meta_pb->set_index_name("test_candidate_full_cover"); + index_meta_pb->clear_col_unique_id(); + index_meta_pb->add_col_unique_id(1); + index_meta_pb->mutable_properties()->insert({"parser", "english"}); + index_meta_pb->mutable_properties()->insert({"lower_case", "true"}); + index_meta_pb->mutable_properties()->insert({"support_phrase", "true"}); + idx_meta.init_from_pb(*index_meta_pb.get()); + + std::string index_path_prefix; + prepare_fulltext_index(rowset_id, seg_id, values, &idx_meta, &index_path_prefix); + auto searcher = create_searcher(index_path_prefix, idx_meta); + ASSERT_NE(searcher, nullptr); + + RuntimeState runtime_state; + io::IOContext io_ctx; + + roaring::Roaring candidate; + candidate.addRange(0, 4); + + IndexQueryContextPtr context = std::make_shared(); + context->io_ctx = &io_ctx; + context->runtime_state = &runtime_state; + context->candidate_rows = &candidate; + + PhraseQuery query(searcher, context); + InvertedIndexQueryInfo query_info; + query_info.field_name = L"1"; + query_info.term_infos.emplace_back("big", 0); + query_info.term_infos.emplace_back("red", 1); + query_info.slop = 0; + query.add(query_info); + + roaring::Roaring result; + query.search(result); + EXPECT_EQ(result.cardinality(), 2); + EXPECT_TRUE(result.contains(0)); + EXPECT_TRUE(result.contains(3)); +} + } // namespace doris::segment_v2 \ No newline at end of file From 57c35226d10c5ca3ac43ec984ac3b4aceb8a0469 Mon Sep 17 00:00:00 2001 From: airborne12 Date: Wed, 26 Aug 2026 23:58:57 +0800 Subject: [PATCH 2/5] [improvement](inverted index) Address review: real norm source, refreshed engage gate, safe config domain Addresses the four automated-review findings on the candidate pushdown: 1. Scoring norm source: with a selective candidate as the leapfrog lead, BM25 norms were read from the candidate adapter (constant 1), flattening per-document length normalization. PhraseQuery now pins _norm_source to the first real postings iterator; a red-first test shows two documents of different lengths scored identically under a candidate before the fix and match the unrestricted scores after it. 2. Engage-gate timing: the threshold was sampled only before _apply_inverted_index(), so an entry bitmap above the ratio that indexed predicates then shrank below it never published candidate_rows. The decision is extracted into _refresh_candidate_pushdown() and re-evaluated at the conjunct boundary after column-level index predicates; covered by a threshold-crossing SegmentIterator test (50% entry -> 5% after an indexed predicate must engage). 3. Config domain safety: the ratio is now guarded twice -- a config validator (finite, <= 1.0) plus an std::isfinite/domain check at the use site before the multiply and integer conversion, so a transiently published out-of-domain value can never reach undefined behavior. Covered by a non-finite-ratio test. 4. Default calibration: 0.1 -> 0.3. The motivating workload keeps 22.4% of each segment after short-key pruning, so the previous default never engaged the pushdown for exactly the case it was built for. --- be/src/common/config.cpp | 5 +- .../index/inverted/query/phrase_query.cpp | 9 +- .../index/inverted/query/phrase_query.h | 3 + be/src/storage/segment/segment_iterator.cpp | 39 ++- be/src/storage/segment/segment_iterator.h | 4 + .../inverted/query/phrase_query_test.cpp | 86 ++++++ ...gment_iterator_candidate_pushdown_test.cpp | 262 ++++++++++++++++++ 7 files changed, 394 insertions(+), 14 deletions(-) create mode 100644 be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp diff --git a/be/src/common/config.cpp b/be/src/common/config.cpp index 0089e9b6725d5e..e8ca55c327ce7e 100644 --- a/be/src/common/config.cpp +++ b/be/src/common/config.cpp @@ -25,6 +25,7 @@ #include #include // IWYU pragma: keep +#include #include #include #include // IWYU pragma: keep @@ -1319,7 +1320,9 @@ 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"); +DEFINE_mDouble(inverted_index_candidate_pushdown_ratio, "0.3"); +DEFINE_Validator(inverted_index_candidate_pushdown_ratio, + [](const double v) -> bool { return std::isfinite(v) && v <= 1.0; }); // inverted index match bitmap cache size DEFINE_String(inverted_index_query_cache_limit, "10%"); diff --git a/be/src/storage/index/inverted/query/phrase_query.cpp b/be/src/storage/index/inverted/query/phrase_query.cpp index 7891be64945aa1..04db80bd7819be 100644 --- a/be/src/storage/index/inverted/query/phrase_query.cpp +++ b/be/src/storage/index/inverted/query/phrase_query.cpp @@ -72,6 +72,12 @@ void PhraseQuery::add(const InvertedIndexQueryInfo& query_info) { for (int32_t i = 2; i < _iterators.size(); i++) { _others.emplace_back(&_iterators[i]); } + for (auto& iter : _iterators) { + if (!std::holds_alternative(iter)) { + _norm_source = &iter; + break; + } + } init_similarities(query_info.field_name, is_similarity); } @@ -176,7 +182,8 @@ void PhraseQuery::search_by_skiplist(roaring::Roaring& roaring) { continue; } roaring.add(doc); - int32_t norm = visit_node(*_lead1, Norm {}); + DORIS_CHECK(_norm_source != nullptr); + int32_t norm = visit_node(*_norm_source, Norm {}); float score = _phrase_similarity->score(phrase_freq, static_cast(norm)); _context->collection_similarity->collect(doc, score); diff --git a/be/src/storage/index/inverted/query/phrase_query.h b/be/src/storage/index/inverted/query/phrase_query.h index 759ff6ad3048ca..478d22c5ceeab7 100644 --- a/be/src/storage/index/inverted/query/phrase_query.h +++ b/be/src/storage/index/inverted/query/phrase_query.h @@ -74,6 +74,9 @@ class PhraseQuery : public Query { DISI* _lead1 = nullptr; DISI* _lead2 = nullptr; + // Norm source for scoring: always a real postings iterator, never the + // pushed-down candidate bitmap (whose norm is a meaningless constant). + DISI* _norm_source = nullptr; std::vector _others; std::vector _iterators; diff --git a/be/src/storage/segment/segment_iterator.cpp b/be/src/storage/segment/segment_iterator.cpp index 4001da5889027c..b54a79a4031687 100644 --- a/be/src/storage/segment/segment_iterator.cpp +++ b/be/src/storage/segment/segment_iterator.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -837,18 +838,10 @@ Status SegmentIterator::_get_row_ranges_by_column_conditions() { // 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; - if (candidate_ratio > 0 && - _row_bitmap.cardinality() < - static_cast(static_cast(num_rows()) * - candidate_ratio)) { - _index_query_context->candidate_rows = &_row_bitmap; - } + // evaluation). _row_bitmap only shrinks during the applies + // below, so restricting to its current state stays correct + // for every later conjunct. + _refresh_candidate_pushdown(); } DEFER({ _capture_count_fastpath_hit(); @@ -860,6 +853,10 @@ Status SegmentIterator::_get_row_ranges_by_column_conditions() { if (has_index_in_iterators()) { RETURN_IF_ERROR(_apply_inverted_index()); } + // The column predicates above may have shrunk the bitmap across + // the engage threshold; refresh the handshake at this conjunct + // boundary so the expression conjuncts below still benefit. + _refresh_candidate_pushdown(); // Always apply expr-level index (e.g., search expressions) if we have common_expr_pushdown // This allows search expressions with variant subcolumns to be evaluated even when // the segment doesn't have all subcolumns @@ -1251,6 +1248,24 @@ bool SegmentIterator::_check_apply_by_inverted_index(std::shared_ptrcandidate_rows != nullptr) { + return; + } + // Guard the domain before the multiply: a non-finite or out-of-range + // configured ratio must never reach the floating-to-integer conversion + // (undefined behavior), even if a runtime config update transiently + // publishes a value the validator rejects. + double candidate_ratio = config::inverted_index_candidate_pushdown_ratio; + if (!std::isfinite(candidate_ratio) || candidate_ratio <= 0 || candidate_ratio > 1) { + return; + } + if (_row_bitmap.cardinality() < + static_cast(static_cast(num_rows()) * candidate_ratio)) { + _index_query_context->candidate_rows = &_row_bitmap; + } +} + Status SegmentIterator::_apply_index_expr() { bool enable_ann_index_result_cache = !_opts.runtime_state || diff --git a/be/src/storage/segment/segment_iterator.h b/be/src/storage/segment/segment_iterator.h index dfe2fd842eb7fb..a1de1ea9c4235b 100644 --- a/be/src/storage/segment/segment_iterator.h +++ b/be/src/storage/segment/segment_iterator.h @@ -155,6 +155,10 @@ class SegmentIterator : public RowwiseIterator { bool* continue_apply); [[nodiscard]] Status _apply_ann_topn_predicate(); [[nodiscard]] Status _apply_index_expr(); + // Publish _row_bitmap as IndexQueryContext::candidate_rows when it is + // below the configured engage ratio; refreshed at conjunct boundaries as + // earlier index conjuncts shrink the bitmap. No-op once engaged. + void _refresh_candidate_pushdown(); // G02: true iff answering the single pushed-down MATCH predicate by its // match COUNT alone is indistinguishable from the row-accurate bitmap for // this COUNT_ON_INDEX scan (no deletes, no other filters, full row bitmap, diff --git a/be/test/storage/index/inverted/query/phrase_query_test.cpp b/be/test/storage/index/inverted/query/phrase_query_test.cpp index cb61ee9a9f22d9..21a7f2cfc0bdf8 100644 --- a/be/test/storage/index/inverted/query/phrase_query_test.cpp +++ b/be/test/storage/index/inverted/query/phrase_query_test.cpp @@ -744,6 +744,92 @@ TEST_F(PhraseQueryTest, candidate_rows_restrict_phrase_search) { } } +// Scoring queries must keep real per-document norms when a candidate is +// pushed down: a selective candidate becomes the leapfrog lead, but BM25 +// norms must still come from a real term iterator, so per-document scores are +// identical with and without the candidate (for the documents both runs +// return). +TEST_F(PhraseQueryTest, candidate_rows_keep_scoring_norms) { + std::string_view rowset_id = "test_candidate_norms"; + int seg_id = 0; + + // Different document lengths => different norms => wrong norm source shows + // up as a score difference. + std::vector values = { + Slice("big red"), // doc 0 short + Slice("big red with quite a few extra filler words in this line"), // doc 1 long + Slice("big red medium length document"), // doc 2 medium + Slice("unrelated words only") // doc 3 + }; + + TabletIndex idx_meta; + auto index_meta_pb = std::make_unique(); + index_meta_pb->set_index_type(IndexType::INVERTED); + index_meta_pb->set_index_id(1); + index_meta_pb->set_index_name("test_candidate_norms"); + index_meta_pb->clear_col_unique_id(); + index_meta_pb->add_col_unique_id(1); + index_meta_pb->mutable_properties()->insert({"parser", "english"}); + index_meta_pb->mutable_properties()->insert({"lower_case", "true"}); + index_meta_pb->mutable_properties()->insert({"support_phrase", "true"}); + idx_meta.init_from_pb(*index_meta_pb.get()); + + std::string index_path_prefix; + prepare_fulltext_index(rowset_id, seg_id, values, &idx_meta, &index_path_prefix); + auto searcher = create_searcher(index_path_prefix, idx_meta); + ASSERT_NE(searcher, nullptr); + + RuntimeState runtime_state; + io::IOContext io_ctx; + + InvertedIndexQueryInfo query_info; + query_info.field_name = L"1"; + query_info.term_infos.emplace_back("big", 0); + query_info.term_infos.emplace_back("red", 1); + query_info.slop = 0; + query_info.is_similarity_score = true; + + // Fixed idf/avgdl so BM25 needs no collected statistics; norms still come + // from the index, which is exactly what this test pins down. + class FixedStats : public CollectionStatistics { + public: + float get_or_calculate_idf(const std::wstring&, const std::wstring&) override { + return 1.0F; + } + float get_or_calculate_avg_dl(const std::wstring&) override { return 5.0F; } + }; + + auto run_scoring = [&](const roaring::Roaring* candidate) { + IndexQueryContextPtr context = std::make_shared(); + context->io_ctx = &io_ctx; + context->runtime_state = &runtime_state; + context->collection_statistics = std::make_shared(); + context->collection_similarity = std::make_shared(); + context->candidate_rows = candidate; + PhraseQuery query(searcher, context); + query.add(query_info); + roaring::Roaring result; + query.search(result); + return context->collection_similarity->release_scores(); + }; + + auto baseline = run_scoring(nullptr); + ASSERT_TRUE(baseline.find(0) != baseline.end()); + ASSERT_TRUE(baseline.find(1) != baseline.end()); + + // Candidate {0, 1}: cardinality 2 is below the term doc-freq (3), so the + // candidate iterator becomes the leapfrog lead. + roaring::Roaring candidate; + candidate.add(0); + candidate.add(1); + auto restricted = run_scoring(&candidate); + + ASSERT_TRUE(restricted.find(0) != restricted.end()); + ASSERT_TRUE(restricted.find(1) != restricted.end()); + EXPECT_FLOAT_EQ(restricted.at(0), baseline.at(0)); + EXPECT_FLOAT_EQ(restricted.at(1), baseline.at(1)); +} + // A candidate covering every row must not change the result (equivalence with // the full-segment evaluation). TEST_F(PhraseQueryTest, candidate_rows_full_cover_keeps_results) { diff --git a/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp b/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp new file mode 100644 index 00000000000000..bae6c49f661173 --- /dev/null +++ b/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp @@ -0,0 +1,262 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// White-box tests for the candidate-pushdown handshake in +// SegmentIterator::_get_row_ranges_by_column_conditions: the engage decision +// must be refreshed after earlier index conjuncts shrink the row bitmap (a +// 50% entry bitmap cut to 5% by an indexed predicate must publish +// candidate_rows for the later expression conjuncts), must reject non-finite +// config values outright, and must always reset on exit. Uses the established +// `#define private public` convention of segment_iterator_limit_opt_test.cpp. +#include + +#include +#include +#include +#include + +#include "common/config.h" +#include "common/status.h" +#include "core/data_type/data_type_number.h" +#include "exprs/vexpr.h" +#include "exprs/vexpr_context.h" +#include "runtime/runtime_state.h" +#include "storage/index/index_iterator.h" +#include "storage/index/index_query_context.h" +#include "storage/olap_common.h" +#include "storage/predicate/column_predicate.h" +#include "storage/tablet/tablet_schema.h" + +#if defined(__clang__) +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wkeyword-macro" +#endif +#define private public +#define protected public +#include "storage/segment/segment.h" +#include "storage/segment/segment_iterator.h" +#undef private +#undef protected +#if defined(__clang__) +#pragma clang diagnostic pop +#endif + +namespace doris::segment_v2 { + +namespace { + +// Records the candidate_rows pointer the SegmentIterator exposes at the moment +// expression conjuncts are index-evaluated. +class CapturingExpr : public VExpr { +public: + explicit CapturingExpr(SegmentIterator* iter) : _iter(iter) { + _data_type = std::make_shared(); + } + + const std::string& expr_name() const override { + static const std::string kName = "CapturingExpr"; + return kName; + } + + Status execute(VExprContext*, Block*, int*) const override { return Status::OK(); } + + Status execute_column_impl(VExprContext* context, const Block* block, const Selector* selector, + size_t count, ColumnPtr& result_column) const override { + return Status::OK(); + } + + Status evaluate_inverted_index(VExprContext* context, uint32_t segment_num_rows) override { + _captured = true; + _captured_candidate = _iter->_index_query_context != nullptr + ? _iter->_index_query_context->candidate_rows + : nullptr; + return Status::OK(); + } + + bool captured() const { return _captured; } + const roaring::Roaring* captured_candidate() const { return _captured_candidate; } + +private: + SegmentIterator* _iter; + bool _captured = false; + const roaring::Roaring* _captured_candidate = nullptr; +}; + +// An indexed predicate stub that shrinks the row bitmap to a fixed set, +// standing in for a selective indexed equality applied before the expression +// conjuncts (modeled on MockNestedPredicate of accept_null_predicate_test). +class ShrinkingPredicate : public ColumnPredicate { +public: + ShrinkingPredicate(uint32_t column_id, std::shared_ptr result_bitmap) + : ColumnPredicate(column_id, "mock_col", PrimitiveType::TYPE_INT, false), + _result_bitmap(std::move(result_bitmap)) {} + + PredicateType type() const override { return PredicateType::EQ; } + + Status evaluate(const IndexFieldNameAndTypePair& name_with_type, IndexIterator* iterator, + uint32_t num_rows, roaring::Roaring* bitmap) const override { + *bitmap = *_result_bitmap; + return Status::OK(); + } + + std::shared_ptr clone(uint32_t col_id) const override { + return std::make_shared(col_id, _result_bitmap); + } + +private: + uint16_t _evaluate_inner(const IColumn& column, uint16_t* sel, uint16_t size) const override { + return size; + } + + std::shared_ptr _result_bitmap; +}; + +// Minimal IndexIterator so has_index_in_iterators()/_check_apply_by_inverted_index pass. +class StubIndexIterator : public IndexIterator { +public: + IndexReaderPtr get_reader(IndexReaderType) const override { return nullptr; } + Status read_from_index(const IndexParam&) override { return Status::OK(); } + Status read_null_bitmap(InvertedIndexQueryCacheHandle*) override { return Status::OK(); } + Result has_null() override { return false; } +}; + +TabletSchemaSPtr make_tablet_schema() { + TabletSchemaPB schema_pb; + schema_pb.set_keys_type(KeysType::DUP_KEYS); + auto* col = schema_pb.add_column(); + col->set_unique_id(0); + col->set_name("k0"); + col->set_type("INT"); + col->set_is_key(true); + col->set_is_nullable(false); + auto tablet_schema = std::make_shared(); + tablet_schema->init_from_pb(schema_pb); + return tablet_schema; +} + +std::shared_ptr make_stub_segment(uint32_t num_rows, + const TabletSchemaSPtr& tablet_schema) { + auto seg = std::make_shared(0, RowsetId(), tablet_schema, InvertedIndexFileInfo()); + seg->_num_rows = num_rows; + return seg; +} + +VExprContextSPtr make_capturing_ctx(const std::shared_ptr& expr) { + auto ctx = std::make_shared(expr); + std::vector> index_iters; + std::vector storage_types; + std::unordered_map> status_map; + ColumnIteratorOptions column_iter_opts; + auto index_ctx = std::make_shared(index_iters, storage_types, status_map, + nullptr, nullptr, column_iter_opts); + ctx->set_index_context(index_ctx); + return ctx; +} + +} // namespace + +class SegmentIteratorCandidatePushdownTest : public testing::Test { +protected: + void SetUp() override { + _saved_ratio = config::inverted_index_candidate_pushdown_ratio; + _tablet_schema = make_tablet_schema(); + _segment = make_stub_segment(100, _tablet_schema); + _read_schema = std::make_shared(_tablet_schema->columns()); + _iter = std::make_unique(_segment, _read_schema); + + TQueryOptions query_options; + query_options.__set_enable_inverted_index_query(true); + query_options.__set_enable_fallback_on_missing_inverted_index(true); + _runtime_state.set_query_options(query_options); + + _iter->_opts.runtime_state = &_runtime_state; + _iter->_opts.stats = &_stats; + _iter->_opts.tablet_schema = _tablet_schema; + _iter->_index_query_context = std::make_shared(); + _iter->_index_query_context->stats = &_stats; + _iter->_column_states.resize(_read_schema->num_read_columns()); + _iter->_storage_name_and_type.resize(_read_schema->num_read_columns()); + + _expr = std::make_shared(_iter.get()); + _iter->_common_expr_ctxs_push_down = {make_capturing_ctx(_expr)}; + } + + void TearDown() override { config::inverted_index_candidate_pushdown_ratio = _saved_ratio; } + + void add_shrinking_predicate(std::initializer_list rows) { + auto result = std::make_shared(); + for (uint32_t row : rows) { + result->add(row); + } + _iter->_index_iterators.resize(1); + _iter->_index_iterators[0] = std::make_unique(); + _iter->_col_predicates.emplace_back(std::make_shared(0, result)); + } + + double _saved_ratio = 0; + std::shared_ptr _segment; + std::shared_ptr _tablet_schema; + ReadSchemaSPtr _read_schema; + std::unique_ptr _iter; + RuntimeState _runtime_state; + OlapReaderStatistics _stats; + std::shared_ptr _expr; +}; + +// Entry bitmap below the threshold: candidate_rows is published for the +// expression conjuncts and reset on exit. +TEST_F(SegmentIteratorCandidatePushdownTest, engages_below_threshold_and_resets) { + config::inverted_index_candidate_pushdown_ratio = 0.3; + _iter->_row_bitmap.addRange(0, 5); // 5% of 100 rows + + ASSERT_TRUE(_iter->_get_row_ranges_by_column_conditions().ok()); + + ASSERT_TRUE(_expr->captured()); + EXPECT_EQ(_expr->captured_candidate(), &_iter->_row_bitmap); + EXPECT_EQ(_iter->_index_query_context->candidate_rows, nullptr); +} + +// CIR-style threshold crossing: the entry bitmap (50%) is above the threshold, +// then an indexed predicate shrinks it to 5%. The handshake must be refreshed +// at that conjunct boundary so the later expression conjuncts still get the +// candidate restriction. +TEST_F(SegmentIteratorCandidatePushdownTest, refreshes_after_index_conjuncts_shrink_bitmap) { + config::inverted_index_candidate_pushdown_ratio = 0.3; + _iter->_row_bitmap.addRange(0, 50); // 50% of 100 rows: no engage at entry + add_shrinking_predicate({0, 1, 2, 3, 4}); + + ASSERT_TRUE(_iter->_get_row_ranges_by_column_conditions().ok()); + + ASSERT_TRUE(_expr->captured()); + EXPECT_EQ(_expr->captured_candidate(), &_iter->_row_bitmap); + EXPECT_EQ(_iter->_index_query_context->candidate_rows, nullptr); +} + +// A non-finite configured ratio must never engage the pushdown (the multiply +// and integer conversion would otherwise be undefined behavior). +TEST_F(SegmentIteratorCandidatePushdownTest, non_finite_ratio_never_engages) { + config::inverted_index_candidate_pushdown_ratio = std::numeric_limits::infinity(); + _iter->_row_bitmap.add(0); // 1 row, far below any finite threshold + + ASSERT_TRUE(_iter->_get_row_ranges_by_column_conditions().ok()); + + ASSERT_TRUE(_expr->captured()); + EXPECT_EQ(_expr->captured_candidate(), nullptr); + EXPECT_EQ(_iter->_index_query_context->candidate_rows, nullptr); +} + +} // namespace doris::segment_v2 From cbc7778a1110693593d7e2d5bf2a8d9eb7cd7781 Mon Sep 17 00:00:00 2001 From: airborne12 Date: Thu, 27 Aug 2026 15:25:54 +0800 Subject: [PATCH 3/5] [improvement](inverted index) Address review: consumption-keyed cache policy, compound three-valued safety Round 2/3 review fixes: - Cache inserts are now gated on actual candidate consumption instead of candidate publication. A reply-direction candidate_rows_consumed flag is set only when PhraseQuery joins the candidate into its leapfrog and is re-armed per search (including the untokenized range path that bypasses match_index_search), so non-consuming queries (MATCH_ANY/ALL, term, regexp, single-term phrase, range) keep caching their full-segment bitmaps while a candidate is engaged. - A candidate-restricted TRUE bitmap paired with the full-segment null bitmap could spuriously trigger VCompoundPred's three-valued AND shortcut (NOT(A AND B) with nullable A: a candidate row's FALSE is mis-typed as NULL and the row is dropped). Compound roots are now evaluated with the candidate suppressed in both the conjunct and virtual-column projection loops; top-level single-predicate consumption keeps the restriction, which is exact within the candidate. - The _norm_source invariant check moved out of the per-document scoring loop; it is validated once before the loop when scoring is active. --- be/src/storage/index/index_query_context.h | 9 + .../index/inverted/inverted_index_reader.cpp | 22 +- .../index/inverted/query/phrase_query.cpp | 7 +- be/src/storage/segment/segment_iterator.cpp | 25 ++- .../segment/inverted_index_reader_test.cpp | 205 ++++++++++++++++++ ...gment_iterator_candidate_pushdown_test.cpp | 34 +++ 6 files changed, 294 insertions(+), 8 deletions(-) diff --git a/be/src/storage/index/index_query_context.h b/be/src/storage/index/index_query_context.h index 3d7a7ed5cbea56..e772a7f0b8685a 100644 --- a/be/src/storage/index/index_query_context.h +++ b/be/src/storage/index/index_query_context.h @@ -77,10 +77,19 @@ struct IndexQueryContext { // remaining count as default rows without iterating the row bitmap. bool count_on_index_fastpath_hit = false; + // Reply direction of the candidate handshake. Set by a query iff it DID + // join candidate_rows into its evaluation (PhraseQuery's leapfrog), i.e. + // its result bitmap is partial; reset by the reader before each search. + // Only such a partial result must stay out of the query cache -- a query + // that never consumes the candidate (MATCH_ANY/ALL, term, regexp, single + // term phrase) still computes the full-segment bitmap and stays cacheable. + bool candidate_rows_consumed = false; + // Folds the reply-direction fields a reader wrote on a copy of this context back into it. // Latching (never clearing) is what makes this safe to call for each of several readers. void merge_reader_outputs(const IndexQueryContext& reader_context) { count_on_index_fastpath_hit |= reader_context.count_on_index_fastpath_hit; + candidate_rows_consumed |= reader_context.candidate_rows_consumed; } }; using IndexQueryContextPtr = std::shared_ptr; diff --git a/be/src/storage/index/inverted/inverted_index_reader.cpp b/be/src/storage/index/inverted/inverted_index_reader.cpp index 01b48fc1f95c79..e8641cad7fd62b 100644 --- a/be/src/storage/index/inverted/inverted_index_reader.cpp +++ b/be/src/storage/index/inverted/inverted_index_reader.cpp @@ -347,6 +347,10 @@ Status InvertedIndexReader::match_index_search( context->runtime_state->query_options().inverted_index_compatible_read) { reader->setCompatibleRead(true); } + // Fresh per-search reply: only the query about to run decides whether it + // consumes the candidate set (and thus produces an uncacheable partial + // result); a consumed flag left by an earlier search must not leak in. + context->candidate_rows_consumed = false; try { SCOPED_RAW_TIMER(&context->stats->inverted_index_searcher_search_timer); auto query = QueryFactory::create(query_type, index_searcher, context); @@ -477,9 +481,11 @@ Status FullTextIndexReader::query(const IndexQueryContextPtr& context, RETURN_IF_ERROR(match_index_search(context, query_type, query_info, *searcher_ptr, term_match_bitmap)); term_match_bitmap->runOptimize(); - // A bitmap produced under a candidate restriction is partial and - // must never be cached as the full-segment result. - if (context->candidate_rows == nullptr) { + // Only a bitmap whose query actually joined the candidate set is + // partial and must stay out of the cache; a non-consuming query + // (MATCH_ANY/ALL, term, regexp, single-term phrase) computed the + // full-segment result even while candidate_rows was published. + if (!context->candidate_rows_consumed) { cache->insert(cache_key, term_match_bitmap, &cache_handler); } bit_map = term_match_bitmap; @@ -544,6 +550,10 @@ Status StringTypeInvertedIndexReader::query(const IndexQueryContextPtr& context, query_info.field_name = column_name_ws; query_info.term_infos.emplace_back(search_str, 0); + // Fresh per-search reply (the range-query cases below never pass + // through match_index_search, so a stale consumed flag from an + // earlier fulltext search must be cleared here too). + context->candidate_rows_consumed = false; auto result = std::make_shared(); FulltextIndexSearcherPtr* searcher_ptr = nullptr; InvertedIndexCacheHandle inverted_index_cache_handle; @@ -602,9 +612,11 @@ Status StringTypeInvertedIndexReader::query(const IndexQueryContextPtr& context, "invalid query type when query untokenized inverted index"); } } - // add to cache + // add to cache (unless a candidate-consuming query made it partial) result->runOptimize(); - cache->insert(cache_key, result, &cache_handler); + if (!context->candidate_rows_consumed) { + cache->insert(cache_key, result, &cache_handler); + } bit_map = result; return Status::OK(); diff --git a/be/src/storage/index/inverted/query/phrase_query.cpp b/be/src/storage/index/inverted/query/phrase_query.cpp index 04db80bd7819be..241b6576a3108d 100644 --- a/be/src/storage/index/inverted/query/phrase_query.cpp +++ b/be/src/storage/index/inverted/query/phrase_query.cpp @@ -59,6 +59,7 @@ void PhraseQuery::add(const InvertedIndexQueryInfo& query_info) { // phrase semantics stay with the real term iterators. if (_context->candidate_rows != nullptr) { _iterators.emplace_back(std::make_shared(_context->candidate_rows)); + _context->candidate_rows_consumed = true; } std::sort(_iterators.begin(), _iterators.end(), [](const DISI& a, const DISI& b) { @@ -174,6 +175,11 @@ void PhraseQuery::search(roaring::Roaring& roaring) { } void PhraseQuery::search_by_skiplist(roaring::Roaring& roaring) { + if (_phrase_similarity) { + // _norm_source is fixed once in add(); validate it before the loop so + // the per-document path below stays free of release-mode checks. + DORIS_CHECK(_norm_source != nullptr); + } int32_t doc = 0; while ((doc = do_next(visit_node(*_lead1, NextDoc {}))) != INT32_MAX) { if (_phrase_similarity) { @@ -182,7 +188,6 @@ void PhraseQuery::search_by_skiplist(roaring::Roaring& roaring) { continue; } roaring.add(doc); - DORIS_CHECK(_norm_source != nullptr); int32_t norm = visit_node(*_norm_source, Norm {}); float score = _phrase_similarity->score(phrase_freq, static_cast(norm)); diff --git a/be/src/storage/segment/segment_iterator.cpp b/be/src/storage/segment/segment_iterator.cpp index b54a79a4031687..92af001916c6be 100644 --- a/be/src/storage/segment/segment_iterator.cpp +++ b/be/src/storage/segment/segment_iterator.cpp @@ -1272,8 +1272,29 @@ Status SegmentIterator::_apply_index_expr() { !_opts.runtime_state->query_options().__isset.enable_ann_index_result_cache || _opts.runtime_state->query_options().enable_ann_index_result_cache; + // Three-valued compound shortcuts (VCompoundPred) treat an empty TRUE + // bitmap as a whole-segment fact; a candidate-restricted TRUE bitmap can + // spuriously trigger them and mis-type candidate rows (NOT(A AND B) with + // nullable A: FALSE becomes NULL and the row is dropped). Compound roots + // are therefore evaluated without the candidate; the top-level + // single-predicate consumption stays exact within the candidate. + auto evaluate_without_candidate_for_compound = [&](const VExprContextSPtr& expr_ctx) { + const bool suppress = _index_query_context != nullptr && + _index_query_context->candidate_rows != nullptr && + expr_ctx->root()->node_type() == TExprNodeType::COMPOUND_PRED; + const roaring::Roaring* saved = suppress ? _index_query_context->candidate_rows : nullptr; + if (suppress) { + _index_query_context->candidate_rows = nullptr; + } + Status st = expr_ctx->evaluate_inverted_index(num_rows()); + if (suppress) { + _index_query_context->candidate_rows = saved; + } + return st; + }; + for (const auto& expr_ctx : _common_expr_ctxs_push_down) { - if (Status st = expr_ctx->evaluate_inverted_index(num_rows()); !st.ok()) { + if (Status st = evaluate_without_candidate_for_compound(expr_ctx); !st.ok()) { if (_downgrade_without_index(st) || st.code() == ErrorCode::NOT_IMPLEMENTED_ERROR) { continue; } else { @@ -1293,7 +1314,7 @@ Status SegmentIterator::_apply_index_expr() { if (expr_ctx->get_index_context() == nullptr) { continue; } - if (Status st = expr_ctx->evaluate_inverted_index(num_rows()); !st.ok()) { + if (Status st = evaluate_without_candidate_for_compound(expr_ctx); !st.ok()) { if (_downgrade_without_index(st) || st.code() == ErrorCode::NOT_IMPLEMENTED_ERROR) { continue; } else { diff --git a/be/test/storage/segment/inverted_index_reader_test.cpp b/be/test/storage/segment/inverted_index_reader_test.cpp index 6d560a1abb1139..e6dbeefed6f5d4 100644 --- a/be/test/storage/segment/inverted_index_reader_test.cpp +++ b/be/test/storage/segment/inverted_index_reader_test.cpp @@ -2426,6 +2426,203 @@ class InvertedIndexReaderTest : public testing::Test { } } + // Candidate-pushdown cache policy: only a query that actually joined the + // candidate bitmap into its evaluation (multi-term phrase) produces a + // partial result that must stay out of the query cache. A query that never + // consumes the candidate (MATCH_ANY here) still computes the full-segment + // bitmap, and a cold miss must keep filling the cache even while + // candidate_rows is published on the context. + void test_candidate_pushdown_cache_policy() { + std::string_view rowset_id = "test_candidate_cache_policy"; + int seg_id = 0; + + std::vector values = { + Slice("the quick brown fox jumps over the lazy dog"), + Slice("apache doris is a fast analytical database"), + Slice("inverted index provides fast text search capabilities")}; + + TabletIndex idx_meta; + auto index_meta_pb = std::make_unique(); + index_meta_pb->set_index_type(IndexType::INVERTED); + index_meta_pb->set_index_id(1); + index_meta_pb->set_index_name("test_candidate_cache_policy"); + index_meta_pb->clear_col_unique_id(); + index_meta_pb->add_col_unique_id(1); + index_meta_pb->mutable_properties()->insert({"parser", "english"}); + index_meta_pb->mutable_properties()->insert({"lower_case", "true"}); + index_meta_pb->mutable_properties()->insert({"support_phrase", "true"}); + idx_meta.init_from_pb(*index_meta_pb.get()); + + std::string index_path_prefix; + prepare_string_index(rowset_id, seg_id, values, &idx_meta, &index_path_prefix); + + OlapReaderStatistics stats; + RuntimeState runtime_state; + TQueryOptions query_options; + query_options.enable_inverted_index_query_cache = true; + query_options.enable_inverted_index_searcher_cache = false; + query_options.inverted_index_max_expansions = 50; + runtime_state.set_query_options(query_options); + + auto reader = std::make_shared( + io::global_local_filesystem(), index_path_prefix, InvertedIndexStorageFormatPB::V2); + EXPECT_TRUE(reader->init().ok()); + auto fulltext_reader = FullTextIndexReader::create_shared(&idx_meta, reader); + EXPECT_NE(fulltext_reader, nullptr); + + io::IOContext io_ctx; + IndexQueryContextPtr context = std::make_shared(); + context->io_ctx = &io_ctx; + context->stats = &stats; + context->runtime_state = &runtime_state; + + roaring::Roaring candidate; + candidate.add(0); + candidate.add(1); + context->candidate_rows = &candidate; + + // MATCH_ANY never consumes the candidate: full-segment result, cacheable. + { + Field qp = Field::create_field(std::string("quick database")); + + std::shared_ptr first = std::make_shared(); + auto status = fulltext_reader->query(context, "1", qp, + InvertedIndexQueryType::MATCH_ANY_QUERY, first); + EXPECT_TRUE(status.ok()) << status; + EXPECT_GT(first->cardinality(), 0); + + std::shared_ptr second = std::make_shared(); + status = fulltext_reader->query(context, "1", qp, + InvertedIndexQueryType::MATCH_ANY_QUERY, second); + EXPECT_TRUE(status.ok()) << status; + EXPECT_EQ(stats.inverted_index_query_cache_hit, 1) + << "the full-segment result of a non-consuming query must be cached " + "even while candidate_rows is published"; + EXPECT_EQ(*first, *second); + } + + // A multi-term phrase joins the candidate into its leapfrog: its result + // is partial and must never be inserted into the cache. + { + Field qp = Field::create_field(std::string("quick brown")); + + std::shared_ptr first = std::make_shared(); + auto status = fulltext_reader->query(context, "1", qp, + InvertedIndexQueryType::MATCH_PHRASE_QUERY, first); + EXPECT_TRUE(status.ok()) << status; + EXPECT_EQ(first->cardinality(), 1); + EXPECT_TRUE(first->contains(0)); + + std::shared_ptr second = std::make_shared(); + status = fulltext_reader->query(context, "1", qp, + InvertedIndexQueryType::MATCH_PHRASE_QUERY, second); + EXPECT_TRUE(status.ok()) << status; + EXPECT_EQ(stats.inverted_index_query_cache_hit, 1) + << "a candidate-restricted phrase result must not be served from or " + "inserted into the query cache"; + EXPECT_EQ(*first, *second); + } + + context->candidate_rows = nullptr; + } + + // The consumed flag must be re-armed per search: a range query on the + // untokenized reader never passes through match_index_search, so a stale + // flag left by an earlier candidate-consuming phrase search must not + // block its (full-segment) result from entering the cache. + void test_candidate_consumed_flag_reset_between_readers() { + std::vector fulltext_values = {Slice("the quick brown fox")}; + TabletIndex fulltext_meta; + auto fulltext_meta_pb = std::make_unique(); + fulltext_meta_pb->set_index_type(IndexType::INVERTED); + fulltext_meta_pb->set_index_id(1); + fulltext_meta_pb->set_index_name("test_consumed_reset_ft"); + fulltext_meta_pb->clear_col_unique_id(); + fulltext_meta_pb->add_col_unique_id(1); + fulltext_meta_pb->mutable_properties()->insert({"parser", "english"}); + fulltext_meta_pb->mutable_properties()->insert({"support_phrase", "true"}); + fulltext_meta.init_from_pb(*fulltext_meta_pb.get()); + std::string fulltext_prefix; + prepare_string_index("test_consumed_reset_ft", 0, fulltext_values, &fulltext_meta, + &fulltext_prefix); + + std::vector plain_values = {Slice("alpha"), Slice("beta")}; + TabletIndex plain_meta; + auto plain_meta_pb = std::make_unique(); + plain_meta_pb->set_index_type(IndexType::INVERTED); + plain_meta_pb->set_index_id(2); + plain_meta_pb->set_index_name("test_consumed_reset_plain"); + plain_meta_pb->clear_col_unique_id(); + plain_meta_pb->add_col_unique_id(1); + plain_meta.init_from_pb(*plain_meta_pb.get()); + std::string plain_prefix; + prepare_string_index("test_consumed_reset_plain", 0, plain_values, &plain_meta, + &plain_prefix); + + OlapReaderStatistics stats; + RuntimeState runtime_state; + TQueryOptions query_options; + query_options.enable_inverted_index_query_cache = true; + query_options.enable_inverted_index_searcher_cache = false; + query_options.inverted_index_max_expansions = 50; + runtime_state.set_query_options(query_options); + + io::IOContext io_ctx; + IndexQueryContextPtr context = std::make_shared(); + context->io_ctx = &io_ctx; + context->stats = &stats; + context->runtime_state = &runtime_state; + + roaring::Roaring candidate; + candidate.add(0); + context->candidate_rows = &candidate; + + // 1) A consuming phrase search on the fulltext reader sets the flag. + { + auto reader = std::make_shared(io::global_local_filesystem(), + fulltext_prefix, + InvertedIndexStorageFormatPB::V2); + EXPECT_TRUE(reader->init().ok()); + auto fulltext_reader = FullTextIndexReader::create_shared(&fulltext_meta, reader); + std::shared_ptr bitmap = std::make_shared(); + Field qp = Field::create_field(std::string("quick brown")); + EXPECT_TRUE(fulltext_reader + ->query(context, "1", qp, + InvertedIndexQueryType::MATCH_PHRASE_QUERY, bitmap) + .ok()); + } + + // 2) A range query on the untokenized reader takes the switch branch + // that bypasses match_index_search; its full-segment result must + // still be cached (second run hits). + { + auto reader = std::make_shared( + io::global_local_filesystem(), plain_prefix, InvertedIndexStorageFormatPB::V2); + EXPECT_TRUE(reader->init().ok()); + auto plain_reader = StringTypeInvertedIndexReader::create_shared(&plain_meta, reader); + Field qp = Field::create_field(std::string("alpha")); + + std::shared_ptr first = std::make_shared(); + EXPECT_TRUE(plain_reader + ->query(context, "1", qp, + InvertedIndexQueryType::GREATER_EQUAL_QUERY, first) + .ok()); + EXPECT_EQ(first->cardinality(), 2); + + std::shared_ptr second = std::make_shared(); + EXPECT_TRUE(plain_reader + ->query(context, "1", qp, + InvertedIndexQueryType::GREATER_EQUAL_QUERY, second) + .ok()); + EXPECT_EQ(stats.inverted_index_query_cache_hit, 1) + << "a stale consumed flag from the earlier phrase search must not " + "block caching of the range query's full-segment result"; + EXPECT_EQ(*first, *second); + } + + context->candidate_rows = nullptr; + } + // Test fulltext index with comprehensive query types void test_fulltext_comprehensive_queries() { std::string_view rowset_id = "test_fulltext_comprehensive"; @@ -4320,6 +4517,14 @@ TEST_F(InvertedIndexReaderTest, UnsupportedDataTypes) { test_unsupported_data_types(); } +TEST_F(InvertedIndexReaderTest, CandidatePushdownCachePolicy) { + test_candidate_pushdown_cache_policy(); +} + +TEST_F(InvertedIndexReaderTest, CandidateConsumedFlagResetBetweenReaders) { + test_candidate_consumed_flag_reset_between_readers(); +} + // Test InvertedIndexResultBitmap operator|= with NULL handling TEST_F(InvertedIndexReaderTest, ResultBitmapOrOperatorNullHandling) { // Test SQL three-valued logic for OR: diff --git a/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp b/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp index bae6c49f661173..0783b5b1661761 100644 --- a/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp +++ b/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp @@ -246,6 +246,40 @@ TEST_F(SegmentIteratorCandidatePushdownTest, refreshes_after_index_conjuncts_shr EXPECT_EQ(_iter->_index_query_context->candidate_rows, nullptr); } +// Three-valued compound shortcuts (VCompoundPred) treat an empty TRUE bitmap +// as a whole-segment fact; under a candidate restriction that inference is +// wrong for candidate rows (NOT(A AND B) with nullable A can turn FALSE into +// NULL and drop rows whose SQL result is TRUE). Compound roots must therefore +// be evaluated without the candidate -- in the conjunct loop and the +// virtual-column projection loop alike -- while simple roots keep it. +TEST_F(SegmentIteratorCandidatePushdownTest, compound_root_evaluates_without_candidate) { + config::inverted_index_candidate_pushdown_ratio = 0.3; + _iter->_row_bitmap.addRange(0, 5); // 5% of 100 rows: candidate engages + + auto compound_expr = std::make_shared(_iter.get()); + compound_expr->set_node_type(TExprNodeType::COMPOUND_PRED); + _iter->_common_expr_ctxs_push_down.push_back(make_capturing_ctx(compound_expr)); + + auto vcol_compound_expr = std::make_shared(_iter.get()); + vcol_compound_expr->set_node_type(TExprNodeType::COMPOUND_PRED); + _iter->_virtual_column_exprs[0] = make_capturing_ctx(vcol_compound_expr); + + ASSERT_TRUE(_iter->_get_row_ranges_by_column_conditions().ok()); + + ASSERT_TRUE(_expr->captured()); + EXPECT_EQ(_expr->captured_candidate(), &_iter->_row_bitmap) + << "a simple root stays candidate-restricted"; + ASSERT_TRUE(compound_expr->captured()); + EXPECT_EQ(compound_expr->captured_candidate(), nullptr) + << "a candidate-restricted TRUE bitmap must not feed three-valued " + "compound shortcuts"; + ASSERT_TRUE(vcol_compound_expr->captured()); + EXPECT_EQ(vcol_compound_expr->captured_candidate(), nullptr) + << "the virtual-column projection loop must suppress the candidate " + "for compound roots too"; + EXPECT_EQ(_iter->_index_query_context->candidate_rows, nullptr); +} + // A non-finite configured ratio must never engage the pushdown (the multiply // and integer conversion would otherwise be undefined behavior). TEST_F(SegmentIteratorCandidatePushdownTest, non_finite_ratio_never_engages) { From f82c48c054d6cdabee6d949c4be4beeb81848ae1 Mon Sep 17 00:00:00 2001 From: airborne12 Date: Sun, 30 Aug 2026 22:53:53 +0800 Subject: [PATCH 4/5] [fix](inverted index) Fix compound candidate safety and phrase-prefix scoring ### What problem does this PR solve? Issue Number: None Related PR: #67180 Problem Summary: A VirtualSlotRef-wrapped compound expression bypassed the candidate suppression used to protect SQL three-valued bitmap evaluation. A nullable NOT(A AND B) expression could therefore short-circuit on a partial TRUE bitmap and silently drop a row for which NOT(NULL AND FALSE) is TRUE. Resolve the effective root through VirtualSlotRef before deciding whether to suppress the candidate. Scoring a multi-term phrase-prefix query could also choose a low-frequency UnionTermIterator as the norm source and fail with "UnionTermIterator does not support scoring". Restrict the norm source to the exact TermPositionsIterator, which owns the per-document norm. A no-candidate run reproduces the same failure, confirming that this scoring edge predates candidate pushdown. ### Release note Fix nullable compound inverted-index filtering through virtual columns and multi-term phrase-prefix scoring failures. ### Check List (For Author) - Test: Unit Test - Added RED/GREEN tests for VirtualSlotRef-wrapped nullable three-valued logic. - Added RED/GREEN real-index scoring tests with and without candidate rows. - Ran 15 related ASAN BE unit tests. - Ran ./build.sh --be with BUILD_TYPE=ASAN. - Behavior changed: Yes. Correct query results are preserved and affected scoring queries no longer fail. - Does this need documentation: No --- .../index/inverted/query/phrase_query.cpp | 8 +- .../index/inverted/query/phrase_query.h | 8 +- be/src/storage/segment/segment_iterator.cpp | 11 +- .../query/phrase_prefix_query_test.cpp | 85 ++++++++++++- ...gment_iterator_candidate_pushdown_test.cpp | 119 ++++++++++++++++++ 5 files changed, 221 insertions(+), 10 deletions(-) diff --git a/be/src/storage/index/inverted/query/phrase_query.cpp b/be/src/storage/index/inverted/query/phrase_query.cpp index 241b6576a3108d..6d853fea08a6c2 100644 --- a/be/src/storage/index/inverted/query/phrase_query.cpp +++ b/be/src/storage/index/inverted/query/phrase_query.cpp @@ -74,8 +74,8 @@ void PhraseQuery::add(const InvertedIndexQueryInfo& query_info) { _others.emplace_back(&_iterators[i]); } for (auto& iter : _iterators) { - if (!std::holds_alternative(iter)) { - _norm_source = &iter; + if (const auto* term_iter = std::get_if(&iter)) { + _norm_source = term_iter->get(); break; } } @@ -188,7 +188,7 @@ void PhraseQuery::search_by_skiplist(roaring::Roaring& roaring) { continue; } roaring.add(doc); - int32_t norm = visit_node(*_norm_source, Norm {}); + int32_t norm = _norm_source->norm(); float score = _phrase_similarity->score(phrase_freq, static_cast(norm)); _context->collection_similarity->collect(doc, score); @@ -305,4 +305,4 @@ void PhraseQuery::parser_info(OlapReaderStatistics* stats, std::string& query, } } -} // namespace doris::segment_v2 \ No newline at end of file +} // namespace doris::segment_v2 diff --git a/be/src/storage/index/inverted/query/phrase_query.h b/be/src/storage/index/inverted/query/phrase_query.h index 478d22c5ceeab7..50811d284e0c31 100644 --- a/be/src/storage/index/inverted/query/phrase_query.h +++ b/be/src/storage/index/inverted/query/phrase_query.h @@ -74,9 +74,9 @@ class PhraseQuery : public Query { DISI* _lead1 = nullptr; DISI* _lead2 = nullptr; - // Norm source for scoring: always a real postings iterator, never the - // pushed-down candidate bitmap (whose norm is a meaningless constant). - DISI* _norm_source = nullptr; + // Norm source for scoring: only an exact term iterator owns per-document + // norms. Candidate and multi-term union iterators remain approximations. + TermPositionsIterator* _norm_source = nullptr; std::vector _others; std::vector _iterators; @@ -85,4 +85,4 @@ class PhraseQuery : public Query { SimilarityPtr _phrase_similarity; }; -} // namespace doris::segment_v2 \ No newline at end of file +} // namespace doris::segment_v2 diff --git a/be/src/storage/segment/segment_iterator.cpp b/be/src/storage/segment/segment_iterator.cpp index 92af001916c6be..ac59ac5b567774 100644 --- a/be/src/storage/segment/segment_iterator.cpp +++ b/be/src/storage/segment/segment_iterator.cpp @@ -1279,9 +1279,18 @@ Status SegmentIterator::_apply_index_expr() { // are therefore evaluated without the candidate; the top-level // single-predicate consumption stays exact within the candidate. auto evaluate_without_candidate_for_compound = [&](const VExprContextSPtr& expr_ctx) { + const auto& root = expr_ctx->root(); + DORIS_CHECK(root != nullptr); + const VExpr* effective_root = root.get(); + if (root->is_virtual_slot_ref()) { + const auto& virtual_expr = + assert_cast(root.get())->get_virtual_column_expr(); + DORIS_CHECK(virtual_expr != nullptr); + effective_root = virtual_expr.get(); + } const bool suppress = _index_query_context != nullptr && _index_query_context->candidate_rows != nullptr && - expr_ctx->root()->node_type() == TExprNodeType::COMPOUND_PRED; + effective_root->node_type() == TExprNodeType::COMPOUND_PRED; const roaring::Roaring* saved = suppress ? _index_query_context->candidate_rows : nullptr; if (suppress) { _index_query_context->candidate_rows = nullptr; diff --git a/be/test/storage/index/inverted/query/phrase_prefix_query_test.cpp b/be/test/storage/index/inverted/query/phrase_prefix_query_test.cpp index 739720d61bd9d2..94b0eca42b3595 100644 --- a/be/test/storage/index/inverted/query/phrase_prefix_query_test.cpp +++ b/be/test/storage/index/inverted/query/phrase_prefix_query_test.cpp @@ -23,11 +23,13 @@ #include "io/fs/local_file_system.h" #include "runtime/exec_env.h" +#include "storage/compaction/collection_similarity.h" #include "storage/index/index_file_reader.h" #include "storage/index/index_file_writer.h" #include "storage/index/inverted/inverted_index_cache.h" #include "storage/index/inverted/inverted_index_searcher.h" #include "storage/index/inverted/inverted_index_writer.h" +#include "storage/index/inverted/similarity/collection_statistics.h" #include "storage/tablet/tablet_schema.h" #include "util/slice.h" @@ -311,6 +313,87 @@ TEST_F(PhrasePrefixQueryTest, test_multi_term_phrase_prefix_query) { EXPECT_GE(result.cardinality(), 0); } +TEST_F(PhrasePrefixQueryTest, scoring_uses_norm_capable_exact_term_iterator) { + std::string_view rowset_id = "test_scoring_norm_source"; + int seg_id = 0; + + // "common" has df=5 while the rare* expansion union has df=2. With a + // one-row candidate the sorted DISI order is candidate, rare* union, + // common exact term, so choosing the first non-candidate iterator as the + // norm source selects a UnionTermIterator that cannot provide norms. + std::vector values = {Slice("common rareone"), Slice("common raretwo"), + Slice("common filler"), Slice("common filler"), + Slice("common filler")}; + + TabletIndex idx_meta; + auto index_meta_pb = std::make_unique(); + index_meta_pb->set_index_type(IndexType::INVERTED); + index_meta_pb->set_index_id(1); + index_meta_pb->set_index_name("test_scoring_norm_source"); + index_meta_pb->clear_col_unique_id(); + index_meta_pb->add_col_unique_id(1); + index_meta_pb->mutable_properties()->insert({"parser", "english"}); + index_meta_pb->mutable_properties()->insert({"lower_case", "true"}); + index_meta_pb->mutable_properties()->insert({"support_phrase", "true"}); + idx_meta.init_from_pb(*index_meta_pb); + + std::string index_path_prefix; + prepare_fulltext_index(rowset_id, seg_id, values, &idx_meta, &index_path_prefix); + auto searcher = create_searcher(index_path_prefix, idx_meta); + ASSERT_NE(searcher, nullptr); + + class FixedStats : public CollectionStatistics { + public: + float get_or_calculate_idf(const std::wstring&, const std::wstring&) override { + return 1.0F; + } + float get_or_calculate_avg_dl(const std::wstring&) override { return 2.0F; } + }; + + RuntimeState runtime_state; + TQueryOptions query_options; + query_options.inverted_index_max_expansions = 50; + runtime_state.set_query_options(query_options); + io::IOContext io_ctx; + roaring::Roaring candidate; + candidate.add(0); + + InvertedIndexQueryInfo query_info; + query_info.field_name = L"1"; + query_info.term_infos.emplace_back("common", 0); + query_info.term_infos.emplace_back("rare", 1); + query_info.is_similarity_score = true; + + auto run_scoring = [&](const roaring::Roaring* candidate_rows) { + IndexQueryContextPtr context = std::make_shared(); + context->io_ctx = &io_ctx; + context->runtime_state = &runtime_state; + context->collection_statistics = std::make_shared(); + context->collection_similarity = std::make_shared(); + context->candidate_rows = candidate_rows; + + PhrasePrefixQuery query(searcher, context); + query.add(query_info); + roaring::Roaring result; + query.search(result); + return std::pair {std::move(result), context->collection_similarity->release_scores()}; + }; + + // The low-df union is also first without a candidate. This pins down that + // the scoring failure predates candidate pushdown rather than attributing + // it to the optimization merely because both share the DISI ordering. + auto [full_result, full_scores] = run_scoring(nullptr); + EXPECT_EQ(full_result.cardinality(), 2); + EXPECT_TRUE(full_result.contains(0)); + EXPECT_TRUE(full_result.contains(1)); + EXPECT_TRUE(full_scores.contains(0)); + EXPECT_TRUE(full_scores.contains(1)); + + auto [restricted_result, restricted_scores] = run_scoring(&candidate); + EXPECT_EQ(restricted_result, candidate); + EXPECT_TRUE(restricted_scores.contains(0)); +} + TEST_F(PhrasePrefixQueryTest, test_empty_terms_exception) { std::string_view rowset_id = "test_empty_terms"; int seg_id = 0; @@ -519,4 +602,4 @@ TEST_F(PhrasePrefixQueryTest, test_phrase_with_no_prefix_expansion) { EXPECT_GE(result.cardinality(), 0); } -} // namespace doris::segment_v2 \ No newline at end of file +} // namespace doris::segment_v2 diff --git a/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp b/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp index 0783b5b1661761..0c3e1f7c05a831 100644 --- a/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp +++ b/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp @@ -32,8 +32,10 @@ #include "common/config.h" #include "common/status.h" #include "core/data_type/data_type_number.h" +#include "exprs/vcompound_pred.h" #include "exprs/vexpr.h" #include "exprs/vexpr_context.h" +#include "exprs/virtual_slot_ref.h" #include "runtime/runtime_state.h" #include "storage/index/index_iterator.h" #include "storage/index/index_query_context.h" @@ -96,6 +98,55 @@ class CapturingExpr : public VExpr { const roaring::Roaring* _captured_candidate = nullptr; }; +// Produces a deterministic inverted-index result while modeling the contract +// of a candidate-consuming leaf: only its TRUE bitmap is candidate-restricted; +// its NULL bitmap remains segment-wide so compound SQL three-valued logic can +// distinguish FALSE from UNKNOWN. +class CandidateRestrictedBitmapExpr : public VExpr { +public: + CandidateRestrictedBitmapExpr(SegmentIterator* iter, std::initializer_list true_rows, + std::initializer_list null_rows) + : _iter(iter) { + _data_type = make_nullable(std::make_shared()); + for (uint32_t row : true_rows) { + _true_rows.add(row); + } + for (uint32_t row : null_rows) { + _null_rows.add(row); + } + } + + const std::string& expr_name() const override { + static const std::string kName = "CandidateRestrictedBitmapExpr"; + return kName; + } + + Status execute_column_impl(VExprContext*, const Block*, const Selector*, size_t, + ColumnPtr&) const override { + return Status::NotSupported("bitmap-only test expression"); + } + + Status evaluate_inverted_index(VExprContext* context, uint32_t) override { + _evaluated = true; + auto data = std::make_shared(_true_rows); + if (_iter->_index_query_context->candidate_rows != nullptr) { + *data &= *_iter->_index_query_context->candidate_rows; + } + context->get_index_context()->set_index_result_for_expr( + this, InvertedIndexResultBitmap(std::move(data), + std::make_shared(_null_rows))); + return Status::OK(); + } + + bool evaluated() const { return _evaluated; } + +private: + SegmentIterator* _iter; + roaring::Roaring _true_rows; + roaring::Roaring _null_rows; + bool _evaluated = false; +}; + // An indexed predicate stub that shrinks the row bitmap to a fixed set, // standing in for a selective indexed equality applied before the expression // conjuncts (modeled on MockNestedPredicate of accept_null_predicate_test). @@ -167,6 +218,44 @@ VExprContextSPtr make_capturing_ctx(const std::shared_ptr& expr) return ctx; } +TExprNode make_compound_node(TExprOpcode::type opcode, int num_children) { + TExprNode node; + node.__set_type(create_type_desc(PrimitiveType::TYPE_BOOLEAN)); + node.__set_node_type(TExprNodeType::COMPOUND_PRED); + node.__set_opcode(opcode); + node.__set_num_children(num_children); + node.__set_is_nullable(true); + return node; +} + +VExprContextSPtr make_virtual_slot_ctx(const VExprSPtr& virtual_expr) { + TExprNode node; + node.__set_type(create_type_desc(PrimitiveType::TYPE_BOOLEAN)); + node.__set_node_type(TExprNodeType::VIRTUAL_SLOT_REF); + node.__set_num_children(0); + node.__set_is_nullable(true); + node.__set_label("virtual_compound"); + TSlotRef slot_ref; + slot_ref.__set_slot_id(-1); + slot_ref.__set_tuple_id(-1); + node.__set_slot_ref(slot_ref); + + auto root = VirtualSlotRef::create_shared(node); + root->set_virtual_column_expr(virtual_expr); + static const std::string kColumnName = "virtual_compound"; + root->set_column_name(&kColumnName); + root->set_column_data_type(make_nullable(std::make_shared())); + + auto ctx = std::make_shared(root); + std::vector> index_iters; + std::vector storage_types; + std::unordered_map> status_map; + ColumnIteratorOptions column_iter_opts; + ctx->set_index_context(std::make_shared( + index_iters, storage_types, status_map, nullptr, nullptr, column_iter_opts)); + return ctx; +} + } // namespace class SegmentIteratorCandidatePushdownTest : public testing::Test { @@ -280,6 +369,36 @@ TEST_F(SegmentIteratorCandidatePushdownTest, compound_root_evaluates_without_can EXPECT_EQ(_iter->_index_query_context->candidate_rows, nullptr); } +TEST_F(SegmentIteratorCandidatePushdownTest, + virtual_slot_wrapped_compound_preserves_three_valued_logic) { + config::inverted_index_candidate_pushdown_ratio = 0.3; + _iter->_row_bitmap.add(0); // 1% of 100 rows: candidate engages + + // At row 0, A is NULL and B is FALSE, so SQL requires + // NOT(A AND B) = NOT(FALSE) = TRUE. A also has a TRUE row outside the + // candidate so a candidate-restricted evaluation makes its TRUE bitmap + // empty and exposes VCompoundPred's invalid early exit. + auto nullable_a = std::make_shared( + _iter.get(), std::initializer_list {50}, std::initializer_list {0}); + auto false_b = std::make_shared( + _iter.get(), std::initializer_list {}, std::initializer_list {}); + auto and_expr = VCompoundPred::create_shared(make_compound_node(TExprOpcode::COMPOUND_AND, 2)); + and_expr->add_child(nullable_a); + and_expr->add_child(false_b); + auto not_expr = VCompoundPred::create_shared(make_compound_node(TExprOpcode::COMPOUND_NOT, 1)); + not_expr->add_child(and_expr); + auto ctx = make_virtual_slot_ctx(not_expr); + _iter->_common_expr_ctxs_push_down = {ctx}; + + ASSERT_TRUE(_iter->_get_row_ranges_by_column_conditions().ok()); + + const auto* result = ctx->get_index_context()->get_index_result_for_expr(not_expr.get()); + ASSERT_NE(result, nullptr); + EXPECT_TRUE(false_b->evaluated()) << "AND must evaluate FALSE B after nullable A"; + EXPECT_TRUE(result->get_data_bitmap()->contains(0)) + << "NOT(NULL AND FALSE) must preserve candidate row 0"; +} + // A non-finite configured ratio must never engage the pushdown (the multiply // and integer conversion would otherwise be undefined behavior). TEST_F(SegmentIteratorCandidatePushdownTest, non_finite_ratio_never_engages) { From 4057b24f2ac55ec5e6218220781c5fc5d4b8925d Mon Sep 17 00:00:00 2001 From: airborne12 Date: Mon, 31 Aug 2026 02:09:41 +0800 Subject: [PATCH 5/5] [fix](inverted index) Include late scan restrictions in phrase candidates ### What problem does this PR solve? Issue Number: None Related PR: #67180 Problem Summary: Candidate pushdown was refreshed before stable scan restrictions were applied. Phrase and phrase-prefix evaluation therefore walked full postings whenever the initial segment bitmap exceeded the engage threshold, even if page pruning, a merge-on-write delete bitmap, or an external scanner split later reduced the scan to a small range. Apply these stable restrictions to the shared row bitmap before inverted-index evaluation so they participate in the candidate gate without changing the final scan domain or condition-cache keying. ### Release note Improve phrase-query candidate pruning for selective scan restrictions. ### Check List (For Author) - Test: Unit Test - Added RED/GREEN cold-cache tests for condition ranges, delete bitmaps, and external row ranges. - Ran 18 related ASAN BE unit tests. - Ran clang-format v16 and check-format. - Behavior changed: Yes. Phrase-family index evaluation can consume stable late-pruning restrictions as candidates. - Does this need documentation: No --- be/src/storage/segment/segment_iterator.cpp | 59 +++++++------ ...gment_iterator_candidate_pushdown_test.cpp | 88 +++++++++++++++++++ 2 files changed, 121 insertions(+), 26 deletions(-) diff --git a/be/src/storage/segment/segment_iterator.cpp b/be/src/storage/segment/segment_iterator.cpp index ac59ac5b567774..f4199675a73a08 100644 --- a/be/src/storage/segment/segment_iterator.cpp +++ b/be/src/storage/segment/segment_iterator.cpp @@ -553,20 +553,6 @@ Status SegmentIterator::_lazy_init(Block* block) { } RETURN_IF_ERROR(_get_row_ranges_by_column_conditions()); RETURN_IF_ERROR(_vec_init_lazy_materialization()); - // Remove rows that have been marked deleted - if (_opts.delete_bitmap.count(segment_id()) > 0 && - _opts.delete_bitmap.at(segment_id()) != nullptr) { - size_t pre_size = _row_bitmap.cardinality(); - _row_bitmap -= *(_opts.delete_bitmap.at(segment_id())); - _opts.stats->rows_del_by_bitmap += (pre_size - _row_bitmap.cardinality()); - VLOG_DEBUG << "read on segment: " << segment_id() << ", delete bitmap cardinality: " - << _opts.delete_bitmap.at(segment_id())->cardinality() << ", " - << _opts.stats->rows_del_by_bitmap << " rows deleted by bitmap"; - } - - if (!_opts.row_ranges.is_empty()) { - _row_bitmap &= RowRanges::ranges_to_roaring(_opts.row_ranges); - } _prepare_score_column_materialization(); @@ -819,6 +805,38 @@ Status SegmentIterator::_get_row_ranges_by_column_conditions() { return Status::OK(); } + // Apply stable scan restrictions before evaluating inverted-index expressions so + // selective restrictions can also serve as phrase-query candidates. The candidate + // pointer keeps referring to _row_bitmap as later predicates shrink it. + auto delete_bitmap_it = _opts.delete_bitmap.find(segment_id()); + if (delete_bitmap_it != _opts.delete_bitmap.end() && delete_bitmap_it->second != nullptr) { + size_t pre_size = _row_bitmap.cardinality(); + _row_bitmap -= *delete_bitmap_it->second; + _opts.stats->rows_del_by_bitmap += (pre_size - _row_bitmap.cardinality()); + VLOG_DEBUG << "read on segment: " << segment_id() + << ", delete bitmap cardinality: " << delete_bitmap_it->second->cardinality() + << ", " << _opts.stats->rows_del_by_bitmap << " rows deleted by bitmap"; + } + + if (!_opts.row_ranges.is_empty()) { + _row_bitmap &= RowRanges::ranges_to_roaring(_opts.row_ranges); + } + + if (!_row_bitmap.isEmpty() && + (!_opts.topn_filter_source_node_ids.empty() || !_opts.col_id_to_predicates.empty() || + _opts.delete_condition_predicates->num_of_column_predicate() > 0 || + !_common_expr_ctxs_push_down.empty())) { + RowRanges condition_row_ranges = RowRanges::create_single(_segment->num_rows()); + RETURN_IF_ERROR(_get_row_ranges_from_conditions(&condition_row_ranges)); + size_t pre_size = _row_bitmap.cardinality(); + _row_bitmap &= RowRanges::ranges_to_roaring(condition_row_ranges); + _opts.stats->rows_conditions_filtered += (pre_size - _row_bitmap.cardinality()); + } + + if (_row_bitmap.isEmpty()) { + return Status::OK(); + } + { if (_opts.runtime_state && _opts.runtime_state->query_options().enable_inverted_index_query && @@ -908,17 +926,6 @@ Status SegmentIterator::_get_row_ranges_by_column_conditions() { } }) - if (!_row_bitmap.isEmpty() && - (!_opts.topn_filter_source_node_ids.empty() || !_opts.col_id_to_predicates.empty() || - _opts.delete_condition_predicates->num_of_column_predicate() > 0 || - !_common_expr_ctxs_push_down.empty())) { - RowRanges condition_row_ranges = RowRanges::create_single(_segment->num_rows()); - RETURN_IF_ERROR(_get_row_ranges_from_conditions(&condition_row_ranges)); - size_t pre_size = _row_bitmap.cardinality(); - _row_bitmap &= RowRanges::ranges_to_roaring(condition_row_ranges); - _opts.stats->rows_conditions_filtered += (pre_size - _row_bitmap.cardinality()); - } - DBUG_EXECUTE_IF("bloom_filter_must_filter_data", { if (_opts.stats->rows_bf_filtered == 0) { return Status::Error( @@ -1382,7 +1389,7 @@ bool SegmentIterator::_count_on_index_fastpath_safe() const { facts.has_virtual_column_exprs = !_virtual_column_exprs.empty(); facts.has_delete_predicates = _opts.delete_condition_predicates != nullptr && _opts.delete_condition_predicates->num_of_column_predicate() > 0; - // Mirror of the _lazy_init delete-bitmap subtraction: the fast path is only + // Mirror of the pre-index delete-bitmap subtraction: the fast path is only // sound when there is nothing to subtract for THIS segment. const auto delete_bitmap_it = _opts.delete_bitmap.find(segment_id()); facts.segment_delete_bitmap_empty = delete_bitmap_it == _opts.delete_bitmap.end() || diff --git a/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp b/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp index 0c3e1f7c05a831..7a06c4e332cbc5 100644 --- a/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp +++ b/be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include "common/config.h" @@ -40,7 +41,9 @@ #include "storage/index/index_iterator.h" #include "storage/index/index_query_context.h" #include "storage/olap_common.h" +#include "storage/predicate/block_column_predicate.h" #include "storage/predicate/column_predicate.h" +#include "storage/segment/column_reader.h" #include "storage/tablet/tablet_schema.h" #if defined(__clang__) @@ -86,16 +89,23 @@ class CapturingExpr : public VExpr { _captured_candidate = _iter->_index_query_context != nullptr ? _iter->_index_query_context->candidate_rows : nullptr; + if (_captured_candidate != nullptr) { + _captured_candidate_copy = *_captured_candidate; + } return Status::OK(); } bool captured() const { return _captured; } const roaring::Roaring* captured_candidate() const { return _captured_candidate; } + const std::optional& captured_candidate_copy() const { + return _captured_candidate_copy; + } private: SegmentIterator* _iter; bool _captured = false; const roaring::Roaring* _captured_candidate = nullptr; + std::optional _captured_candidate_copy; }; // Produces a deterministic inverted-index result while modeling the contract @@ -185,6 +195,26 @@ class StubIndexIterator : public IndexIterator { Result has_null() override { return false; } }; +class RangePruningColumnIterator : public ColumnIterator { +public: + explicit RangePruningColumnIterator(RowRanges row_ranges) + : _row_ranges(std::move(row_ranges)) {} + + Status seek_to_ordinal(ordinal_t ord) override { return Status::OK(); } + ordinal_t get_current_ordinal() const override { return 0; } + + Status get_row_ranges_by_zone_map( + const AndBlockColumnPredicate* col_predicates, + const std::vector>* delete_predicates, + RowRanges* row_ranges) override { + *row_ranges = _row_ranges; + return Status::OK(); + } + +private: + RowRanges _row_ranges; +}; + TabletSchemaSPtr make_tablet_schema() { TabletSchemaPB schema_pb; schema_pb.set_keys_type(KeysType::DUP_KEYS); @@ -296,6 +326,17 @@ class SegmentIteratorCandidatePushdownTest : public testing::Test { _iter->_col_predicates.emplace_back(std::make_shared(0, result)); } + void add_range_pruning_condition(rowid_t from, rowid_t to) { + _iter->_column_iterators[0] = + std::make_unique(RowRanges::create_single(from, to)); + auto result = std::make_shared(); + auto predicate = std::make_shared(0, std::move(result)); + auto block_predicate = AndBlockColumnPredicate::create_shared(); + block_predicate->add_column_predicate( + SingleColumnBlockPredicate::create_unique(std::move(predicate))); + _iter->_opts.col_id_to_predicates.emplace(0, std::move(block_predicate)); + } + double _saved_ratio = 0; std::shared_ptr _segment; std::shared_ptr _tablet_schema; @@ -335,6 +376,53 @@ TEST_F(SegmentIteratorCandidatePushdownTest, refreshes_after_index_conjuncts_shr EXPECT_EQ(_iter->_index_query_context->candidate_rows, nullptr); } +TEST_F(SegmentIteratorCandidatePushdownTest, external_row_ranges_engage_candidate_before_expr) { + config::inverted_index_candidate_pushdown_ratio = 0.3; + _iter->_row_bitmap.addRange(0, 100); // full segment: no engage without the split + _iter->_opts.row_ranges = RowRanges::create_single(0, 5); + + ASSERT_TRUE(_iter->_get_row_ranges_by_column_conditions().ok()); + + ASSERT_TRUE(_expr->captured()); + ASSERT_TRUE(_expr->captured_candidate_copy().has_value()); + EXPECT_EQ(_expr->captured_candidate_copy()->cardinality(), 5); + EXPECT_TRUE(_expr->captured_candidate_copy()->contains(0)); + EXPECT_FALSE(_expr->captured_candidate_copy()->contains(5)); + EXPECT_EQ(_iter->_index_query_context->candidate_rows, nullptr); +} + +TEST_F(SegmentIteratorCandidatePushdownTest, delete_bitmap_engages_candidate_before_expr) { + config::inverted_index_candidate_pushdown_ratio = 0.3; + _iter->_row_bitmap.addRange(0, 100); // full segment: no engage without deletes + auto deleted_rows = std::make_shared(); + deleted_rows->addRange(5, 100); + _iter->_opts.delete_bitmap.emplace(_iter->segment_id(), std::move(deleted_rows)); + + ASSERT_TRUE(_iter->_get_row_ranges_by_column_conditions().ok()); + + ASSERT_TRUE(_expr->captured()); + ASSERT_TRUE(_expr->captured_candidate_copy().has_value()); + EXPECT_EQ(_expr->captured_candidate_copy()->cardinality(), 5); + EXPECT_TRUE(_expr->captured_candidate_copy()->contains(4)); + EXPECT_FALSE(_expr->captured_candidate_copy()->contains(5)); + EXPECT_EQ(_iter->_index_query_context->candidate_rows, nullptr); +} + +TEST_F(SegmentIteratorCandidatePushdownTest, condition_ranges_engage_candidate_before_expr) { + config::inverted_index_candidate_pushdown_ratio = 0.3; + _iter->_row_bitmap.addRange(0, 100); // full segment: no engage without range pruning + add_range_pruning_condition(0, 5); + + ASSERT_TRUE(_iter->_get_row_ranges_by_column_conditions().ok()); + + ASSERT_TRUE(_expr->captured()); + ASSERT_TRUE(_expr->captured_candidate_copy().has_value()); + EXPECT_EQ(_expr->captured_candidate_copy()->cardinality(), 5); + EXPECT_TRUE(_expr->captured_candidate_copy()->contains(0)); + EXPECT_FALSE(_expr->captured_candidate_copy()->contains(5)); + EXPECT_EQ(_iter->_index_query_context->candidate_rows, nullptr); +} + // Three-valued compound shortcuts (VCompoundPred) treat an empty TRUE bitmap // as a whole-segment fact; under a candidate restriction that inference is // wrong for candidate rows (NOT(A AND B) with nullable A can turn FALSE into