Skip to content

Commit 6c6514b

Browse files
committed
[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.
1 parent 53b393c commit 6c6514b

6 files changed

Lines changed: 294 additions & 8 deletions

File tree

be/src/storage/index/index_query_context.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,19 @@ struct IndexQueryContext {
7777
// remaining count as default rows without iterating the row bitmap.
7878
bool count_on_index_fastpath_hit = false;
7979

80+
// Reply direction of the candidate handshake. Set by a query iff it DID
81+
// join candidate_rows into its evaluation (PhraseQuery's leapfrog), i.e.
82+
// its result bitmap is partial; reset by the reader before each search.
83+
// Only such a partial result must stay out of the query cache -- a query
84+
// that never consumes the candidate (MATCH_ANY/ALL, term, regexp, single
85+
// term phrase) still computes the full-segment bitmap and stays cacheable.
86+
bool candidate_rows_consumed = false;
87+
8088
// Folds the reply-direction fields a reader wrote on a copy of this context back into it.
8189
// Latching (never clearing) is what makes this safe to call for each of several readers.
8290
void merge_reader_outputs(const IndexQueryContext& reader_context) {
8391
count_on_index_fastpath_hit |= reader_context.count_on_index_fastpath_hit;
92+
candidate_rows_consumed |= reader_context.candidate_rows_consumed;
8493
}
8594
};
8695
using IndexQueryContextPtr = std::shared_ptr<IndexQueryContext>;

be/src/storage/index/inverted/inverted_index_reader.cpp

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,10 @@ Status InvertedIndexReader::match_index_search(
347347
context->runtime_state->query_options().inverted_index_compatible_read) {
348348
reader->setCompatibleRead(true);
349349
}
350+
// Fresh per-search reply: only the query about to run decides whether it
351+
// consumes the candidate set (and thus produces an uncacheable partial
352+
// result); a consumed flag left by an earlier search must not leak in.
353+
context->candidate_rows_consumed = false;
350354
try {
351355
SCOPED_RAW_TIMER(&context->stats->inverted_index_searcher_search_timer);
352356
auto query = QueryFactory::create(query_type, index_searcher, context);
@@ -477,9 +481,11 @@ Status FullTextIndexReader::query(const IndexQueryContextPtr& context,
477481
RETURN_IF_ERROR(match_index_search(context, query_type, query_info, *searcher_ptr,
478482
term_match_bitmap));
479483
term_match_bitmap->runOptimize();
480-
// A bitmap produced under a candidate restriction is partial and
481-
// must never be cached as the full-segment result.
482-
if (context->candidate_rows == nullptr) {
484+
// Only a bitmap whose query actually joined the candidate set is
485+
// partial and must stay out of the cache; a non-consuming query
486+
// (MATCH_ANY/ALL, term, regexp, single-term phrase) computed the
487+
// full-segment result even while candidate_rows was published.
488+
if (!context->candidate_rows_consumed) {
483489
cache->insert(cache_key, term_match_bitmap, &cache_handler);
484490
}
485491
bit_map = term_match_bitmap;
@@ -544,6 +550,10 @@ Status StringTypeInvertedIndexReader::query(const IndexQueryContextPtr& context,
544550
query_info.field_name = column_name_ws;
545551
query_info.term_infos.emplace_back(search_str, 0);
546552

553+
// Fresh per-search reply (the range-query cases below never pass
554+
// through match_index_search, so a stale consumed flag from an
555+
// earlier fulltext search must be cleared here too).
556+
context->candidate_rows_consumed = false;
547557
auto result = std::make_shared<roaring::Roaring>();
548558
FulltextIndexSearcherPtr* searcher_ptr = nullptr;
549559
InvertedIndexCacheHandle inverted_index_cache_handle;
@@ -602,9 +612,11 @@ Status StringTypeInvertedIndexReader::query(const IndexQueryContextPtr& context,
602612
"invalid query type when query untokenized inverted index");
603613
}
604614
}
605-
// add to cache
615+
// add to cache (unless a candidate-consuming query made it partial)
606616
result->runOptimize();
607-
cache->insert(cache_key, result, &cache_handler);
617+
if (!context->candidate_rows_consumed) {
618+
cache->insert(cache_key, result, &cache_handler);
619+
}
608620

609621
bit_map = result;
610622
return Status::OK();

be/src/storage/index/inverted/query/phrase_query.cpp

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ void PhraseQuery::add(const InvertedIndexQueryInfo& query_info) {
5959
// phrase semantics stay with the real term iterators.
6060
if (_context->candidate_rows != nullptr) {
6161
_iterators.emplace_back(std::make_shared<RoaringDocIdIterator>(_context->candidate_rows));
62+
_context->candidate_rows_consumed = true;
6263
}
6364

6465
std::sort(_iterators.begin(), _iterators.end(), [](const DISI& a, const DISI& b) {
@@ -174,6 +175,11 @@ void PhraseQuery::search(roaring::Roaring& roaring) {
174175
}
175176

176177
void PhraseQuery::search_by_skiplist(roaring::Roaring& roaring) {
178+
if (_phrase_similarity) {
179+
// _norm_source is fixed once in add(); validate it before the loop so
180+
// the per-document path below stays free of release-mode checks.
181+
DORIS_CHECK(_norm_source != nullptr);
182+
}
177183
int32_t doc = 0;
178184
while ((doc = do_next(visit_node(*_lead1, NextDoc {}))) != INT32_MAX) {
179185
if (_phrase_similarity) {
@@ -182,7 +188,6 @@ void PhraseQuery::search_by_skiplist(roaring::Roaring& roaring) {
182188
continue;
183189
}
184190
roaring.add(doc);
185-
DORIS_CHECK(_norm_source != nullptr);
186191
int32_t norm = visit_node(*_norm_source, Norm {});
187192
float score = _phrase_similarity->score(phrase_freq, static_cast<int64_t>(norm));
188193

be/src/storage/segment/segment_iterator.cpp

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1272,8 +1272,29 @@ Status SegmentIterator::_apply_index_expr() {
12721272
!_opts.runtime_state->query_options().__isset.enable_ann_index_result_cache ||
12731273
_opts.runtime_state->query_options().enable_ann_index_result_cache;
12741274

1275+
// Three-valued compound shortcuts (VCompoundPred) treat an empty TRUE
1276+
// bitmap as a whole-segment fact; a candidate-restricted TRUE bitmap can
1277+
// spuriously trigger them and mis-type candidate rows (NOT(A AND B) with
1278+
// nullable A: FALSE becomes NULL and the row is dropped). Compound roots
1279+
// are therefore evaluated without the candidate; the top-level
1280+
// single-predicate consumption stays exact within the candidate.
1281+
auto evaluate_without_candidate_for_compound = [&](const VExprContextSPtr& expr_ctx) {
1282+
const bool suppress = _index_query_context != nullptr &&
1283+
_index_query_context->candidate_rows != nullptr &&
1284+
expr_ctx->root()->node_type() == TExprNodeType::COMPOUND_PRED;
1285+
const roaring::Roaring* saved = suppress ? _index_query_context->candidate_rows : nullptr;
1286+
if (suppress) {
1287+
_index_query_context->candidate_rows = nullptr;
1288+
}
1289+
Status st = expr_ctx->evaluate_inverted_index(num_rows());
1290+
if (suppress) {
1291+
_index_query_context->candidate_rows = saved;
1292+
}
1293+
return st;
1294+
};
1295+
12751296
for (const auto& expr_ctx : _common_expr_ctxs_push_down) {
1276-
if (Status st = expr_ctx->evaluate_inverted_index(num_rows()); !st.ok()) {
1297+
if (Status st = evaluate_without_candidate_for_compound(expr_ctx); !st.ok()) {
12771298
if (_downgrade_without_index(st) || st.code() == ErrorCode::NOT_IMPLEMENTED_ERROR) {
12781299
continue;
12791300
} else {
@@ -1293,7 +1314,7 @@ Status SegmentIterator::_apply_index_expr() {
12931314
if (expr_ctx->get_index_context() == nullptr) {
12941315
continue;
12951316
}
1296-
if (Status st = expr_ctx->evaluate_inverted_index(num_rows()); !st.ok()) {
1317+
if (Status st = evaluate_without_candidate_for_compound(expr_ctx); !st.ok()) {
12971318
if (_downgrade_without_index(st) || st.code() == ErrorCode::NOT_IMPLEMENTED_ERROR) {
12981319
continue;
12991320
} else {

be/test/storage/segment/inverted_index_reader_test.cpp

Lines changed: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2426,6 +2426,203 @@ class InvertedIndexReaderTest : public testing::Test {
24262426
}
24272427
}
24282428

2429+
// Candidate-pushdown cache policy: only a query that actually joined the
2430+
// candidate bitmap into its evaluation (multi-term phrase) produces a
2431+
// partial result that must stay out of the query cache. A query that never
2432+
// consumes the candidate (MATCH_ANY here) still computes the full-segment
2433+
// bitmap, and a cold miss must keep filling the cache even while
2434+
// candidate_rows is published on the context.
2435+
void test_candidate_pushdown_cache_policy() {
2436+
std::string_view rowset_id = "test_candidate_cache_policy";
2437+
int seg_id = 0;
2438+
2439+
std::vector<Slice> values = {
2440+
Slice("the quick brown fox jumps over the lazy dog"),
2441+
Slice("apache doris is a fast analytical database"),
2442+
Slice("inverted index provides fast text search capabilities")};
2443+
2444+
TabletIndex idx_meta;
2445+
auto index_meta_pb = std::make_unique<TabletIndexPB>();
2446+
index_meta_pb->set_index_type(IndexType::INVERTED);
2447+
index_meta_pb->set_index_id(1);
2448+
index_meta_pb->set_index_name("test_candidate_cache_policy");
2449+
index_meta_pb->clear_col_unique_id();
2450+
index_meta_pb->add_col_unique_id(1);
2451+
index_meta_pb->mutable_properties()->insert({"parser", "english"});
2452+
index_meta_pb->mutable_properties()->insert({"lower_case", "true"});
2453+
index_meta_pb->mutable_properties()->insert({"support_phrase", "true"});
2454+
idx_meta.init_from_pb(*index_meta_pb.get());
2455+
2456+
std::string index_path_prefix;
2457+
prepare_string_index(rowset_id, seg_id, values, &idx_meta, &index_path_prefix);
2458+
2459+
OlapReaderStatistics stats;
2460+
RuntimeState runtime_state;
2461+
TQueryOptions query_options;
2462+
query_options.enable_inverted_index_query_cache = true;
2463+
query_options.enable_inverted_index_searcher_cache = false;
2464+
query_options.inverted_index_max_expansions = 50;
2465+
runtime_state.set_query_options(query_options);
2466+
2467+
auto reader = std::make_shared<IndexFileReader>(
2468+
io::global_local_filesystem(), index_path_prefix, InvertedIndexStorageFormatPB::V2);
2469+
EXPECT_TRUE(reader->init().ok());
2470+
auto fulltext_reader = FullTextIndexReader::create_shared(&idx_meta, reader);
2471+
EXPECT_NE(fulltext_reader, nullptr);
2472+
2473+
io::IOContext io_ctx;
2474+
IndexQueryContextPtr context = std::make_shared<IndexQueryContext>();
2475+
context->io_ctx = &io_ctx;
2476+
context->stats = &stats;
2477+
context->runtime_state = &runtime_state;
2478+
2479+
roaring::Roaring candidate;
2480+
candidate.add(0);
2481+
candidate.add(1);
2482+
context->candidate_rows = &candidate;
2483+
2484+
// MATCH_ANY never consumes the candidate: full-segment result, cacheable.
2485+
{
2486+
Field qp = Field::create_field<TYPE_STRING>(std::string("quick database"));
2487+
2488+
std::shared_ptr<roaring::Roaring> first = std::make_shared<roaring::Roaring>();
2489+
auto status = fulltext_reader->query(context, "1", qp,
2490+
InvertedIndexQueryType::MATCH_ANY_QUERY, first);
2491+
EXPECT_TRUE(status.ok()) << status;
2492+
EXPECT_GT(first->cardinality(), 0);
2493+
2494+
std::shared_ptr<roaring::Roaring> second = std::make_shared<roaring::Roaring>();
2495+
status = fulltext_reader->query(context, "1", qp,
2496+
InvertedIndexQueryType::MATCH_ANY_QUERY, second);
2497+
EXPECT_TRUE(status.ok()) << status;
2498+
EXPECT_EQ(stats.inverted_index_query_cache_hit, 1)
2499+
<< "the full-segment result of a non-consuming query must be cached "
2500+
"even while candidate_rows is published";
2501+
EXPECT_EQ(*first, *second);
2502+
}
2503+
2504+
// A multi-term phrase joins the candidate into its leapfrog: its result
2505+
// is partial and must never be inserted into the cache.
2506+
{
2507+
Field qp = Field::create_field<TYPE_STRING>(std::string("quick brown"));
2508+
2509+
std::shared_ptr<roaring::Roaring> first = std::make_shared<roaring::Roaring>();
2510+
auto status = fulltext_reader->query(context, "1", qp,
2511+
InvertedIndexQueryType::MATCH_PHRASE_QUERY, first);
2512+
EXPECT_TRUE(status.ok()) << status;
2513+
EXPECT_EQ(first->cardinality(), 1);
2514+
EXPECT_TRUE(first->contains(0));
2515+
2516+
std::shared_ptr<roaring::Roaring> second = std::make_shared<roaring::Roaring>();
2517+
status = fulltext_reader->query(context, "1", qp,
2518+
InvertedIndexQueryType::MATCH_PHRASE_QUERY, second);
2519+
EXPECT_TRUE(status.ok()) << status;
2520+
EXPECT_EQ(stats.inverted_index_query_cache_hit, 1)
2521+
<< "a candidate-restricted phrase result must not be served from or "
2522+
"inserted into the query cache";
2523+
EXPECT_EQ(*first, *second);
2524+
}
2525+
2526+
context->candidate_rows = nullptr;
2527+
}
2528+
2529+
// The consumed flag must be re-armed per search: a range query on the
2530+
// untokenized reader never passes through match_index_search, so a stale
2531+
// flag left by an earlier candidate-consuming phrase search must not
2532+
// block its (full-segment) result from entering the cache.
2533+
void test_candidate_consumed_flag_reset_between_readers() {
2534+
std::vector<Slice> fulltext_values = {Slice("the quick brown fox")};
2535+
TabletIndex fulltext_meta;
2536+
auto fulltext_meta_pb = std::make_unique<TabletIndexPB>();
2537+
fulltext_meta_pb->set_index_type(IndexType::INVERTED);
2538+
fulltext_meta_pb->set_index_id(1);
2539+
fulltext_meta_pb->set_index_name("test_consumed_reset_ft");
2540+
fulltext_meta_pb->clear_col_unique_id();
2541+
fulltext_meta_pb->add_col_unique_id(1);
2542+
fulltext_meta_pb->mutable_properties()->insert({"parser", "english"});
2543+
fulltext_meta_pb->mutable_properties()->insert({"support_phrase", "true"});
2544+
fulltext_meta.init_from_pb(*fulltext_meta_pb.get());
2545+
std::string fulltext_prefix;
2546+
prepare_string_index("test_consumed_reset_ft", 0, fulltext_values, &fulltext_meta,
2547+
&fulltext_prefix);
2548+
2549+
std::vector<Slice> plain_values = {Slice("alpha"), Slice("beta")};
2550+
TabletIndex plain_meta;
2551+
auto plain_meta_pb = std::make_unique<TabletIndexPB>();
2552+
plain_meta_pb->set_index_type(IndexType::INVERTED);
2553+
plain_meta_pb->set_index_id(2);
2554+
plain_meta_pb->set_index_name("test_consumed_reset_plain");
2555+
plain_meta_pb->clear_col_unique_id();
2556+
plain_meta_pb->add_col_unique_id(1);
2557+
plain_meta.init_from_pb(*plain_meta_pb.get());
2558+
std::string plain_prefix;
2559+
prepare_string_index("test_consumed_reset_plain", 0, plain_values, &plain_meta,
2560+
&plain_prefix);
2561+
2562+
OlapReaderStatistics stats;
2563+
RuntimeState runtime_state;
2564+
TQueryOptions query_options;
2565+
query_options.enable_inverted_index_query_cache = true;
2566+
query_options.enable_inverted_index_searcher_cache = false;
2567+
query_options.inverted_index_max_expansions = 50;
2568+
runtime_state.set_query_options(query_options);
2569+
2570+
io::IOContext io_ctx;
2571+
IndexQueryContextPtr context = std::make_shared<IndexQueryContext>();
2572+
context->io_ctx = &io_ctx;
2573+
context->stats = &stats;
2574+
context->runtime_state = &runtime_state;
2575+
2576+
roaring::Roaring candidate;
2577+
candidate.add(0);
2578+
context->candidate_rows = &candidate;
2579+
2580+
// 1) A consuming phrase search on the fulltext reader sets the flag.
2581+
{
2582+
auto reader = std::make_shared<IndexFileReader>(io::global_local_filesystem(),
2583+
fulltext_prefix,
2584+
InvertedIndexStorageFormatPB::V2);
2585+
EXPECT_TRUE(reader->init().ok());
2586+
auto fulltext_reader = FullTextIndexReader::create_shared(&fulltext_meta, reader);
2587+
std::shared_ptr<roaring::Roaring> bitmap = std::make_shared<roaring::Roaring>();
2588+
Field qp = Field::create_field<TYPE_STRING>(std::string("quick brown"));
2589+
EXPECT_TRUE(fulltext_reader
2590+
->query(context, "1", qp,
2591+
InvertedIndexQueryType::MATCH_PHRASE_QUERY, bitmap)
2592+
.ok());
2593+
}
2594+
2595+
// 2) A range query on the untokenized reader takes the switch branch
2596+
// that bypasses match_index_search; its full-segment result must
2597+
// still be cached (second run hits).
2598+
{
2599+
auto reader = std::make_shared<IndexFileReader>(
2600+
io::global_local_filesystem(), plain_prefix, InvertedIndexStorageFormatPB::V2);
2601+
EXPECT_TRUE(reader->init().ok());
2602+
auto plain_reader = StringTypeInvertedIndexReader::create_shared(&plain_meta, reader);
2603+
Field qp = Field::create_field<TYPE_STRING>(std::string("alpha"));
2604+
2605+
std::shared_ptr<roaring::Roaring> first = std::make_shared<roaring::Roaring>();
2606+
EXPECT_TRUE(plain_reader
2607+
->query(context, "1", qp,
2608+
InvertedIndexQueryType::GREATER_EQUAL_QUERY, first)
2609+
.ok());
2610+
EXPECT_EQ(first->cardinality(), 2);
2611+
2612+
std::shared_ptr<roaring::Roaring> second = std::make_shared<roaring::Roaring>();
2613+
EXPECT_TRUE(plain_reader
2614+
->query(context, "1", qp,
2615+
InvertedIndexQueryType::GREATER_EQUAL_QUERY, second)
2616+
.ok());
2617+
EXPECT_EQ(stats.inverted_index_query_cache_hit, 1)
2618+
<< "a stale consumed flag from the earlier phrase search must not "
2619+
"block caching of the range query's full-segment result";
2620+
EXPECT_EQ(*first, *second);
2621+
}
2622+
2623+
context->candidate_rows = nullptr;
2624+
}
2625+
24292626
// Test fulltext index with comprehensive query types
24302627
void test_fulltext_comprehensive_queries() {
24312628
std::string_view rowset_id = "test_fulltext_comprehensive";
@@ -4320,6 +4517,14 @@ TEST_F(InvertedIndexReaderTest, UnsupportedDataTypes) {
43204517
test_unsupported_data_types();
43214518
}
43224519

4520+
TEST_F(InvertedIndexReaderTest, CandidatePushdownCachePolicy) {
4521+
test_candidate_pushdown_cache_policy();
4522+
}
4523+
4524+
TEST_F(InvertedIndexReaderTest, CandidateConsumedFlagResetBetweenReaders) {
4525+
test_candidate_consumed_flag_reset_between_readers();
4526+
}
4527+
43234528
// Test InvertedIndexResultBitmap operator|= with NULL handling
43244529
TEST_F(InvertedIndexReaderTest, ResultBitmapOrOperatorNullHandling) {
43254530
// Test SQL three-valued logic for OR:

0 commit comments

Comments
 (0)