Skip to content

Commit f82c48c

Browse files
committed
[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
1 parent cbc7778 commit f82c48c

5 files changed

Lines changed: 221 additions & 10 deletions

File tree

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,8 @@ void PhraseQuery::add(const InvertedIndexQueryInfo& query_info) {
7474
_others.emplace_back(&_iterators[i]);
7575
}
7676
for (auto& iter : _iterators) {
77-
if (!std::holds_alternative<RoaringDocIdIterPtr>(iter)) {
78-
_norm_source = &iter;
77+
if (const auto* term_iter = std::get_if<TermPositionsIterPtr>(&iter)) {
78+
_norm_source = term_iter->get();
7979
break;
8080
}
8181
}
@@ -188,7 +188,7 @@ void PhraseQuery::search_by_skiplist(roaring::Roaring& roaring) {
188188
continue;
189189
}
190190
roaring.add(doc);
191-
int32_t norm = visit_node(*_norm_source, Norm {});
191+
int32_t norm = _norm_source->norm();
192192
float score = _phrase_similarity->score(phrase_freq, static_cast<int64_t>(norm));
193193

194194
_context->collection_similarity->collect(doc, score);
@@ -305,4 +305,4 @@ void PhraseQuery::parser_info(OlapReaderStatistics* stats, std::string& query,
305305
}
306306
}
307307

308-
} // namespace doris::segment_v2
308+
} // namespace doris::segment_v2

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,9 @@ class PhraseQuery : public Query {
7474

7575
DISI* _lead1 = nullptr;
7676
DISI* _lead2 = nullptr;
77-
// Norm source for scoring: always a real postings iterator, never the
78-
// pushed-down candidate bitmap (whose norm is a meaningless constant).
79-
DISI* _norm_source = nullptr;
77+
// Norm source for scoring: only an exact term iterator owns per-document
78+
// norms. Candidate and multi-term union iterators remain approximations.
79+
TermPositionsIterator* _norm_source = nullptr;
8080
std::vector<DISI*> _others;
8181
std::vector<DISI> _iterators;
8282

@@ -85,4 +85,4 @@ class PhraseQuery : public Query {
8585
SimilarityPtr _phrase_similarity;
8686
};
8787

88-
} // namespace doris::segment_v2
88+
} // namespace doris::segment_v2

be/src/storage/segment/segment_iterator.cpp

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1279,9 +1279,18 @@ Status SegmentIterator::_apply_index_expr() {
12791279
// are therefore evaluated without the candidate; the top-level
12801280
// single-predicate consumption stays exact within the candidate.
12811281
auto evaluate_without_candidate_for_compound = [&](const VExprContextSPtr& expr_ctx) {
1282+
const auto& root = expr_ctx->root();
1283+
DORIS_CHECK(root != nullptr);
1284+
const VExpr* effective_root = root.get();
1285+
if (root->is_virtual_slot_ref()) {
1286+
const auto& virtual_expr =
1287+
assert_cast<const VirtualSlotRef*>(root.get())->get_virtual_column_expr();
1288+
DORIS_CHECK(virtual_expr != nullptr);
1289+
effective_root = virtual_expr.get();
1290+
}
12821291
const bool suppress = _index_query_context != nullptr &&
12831292
_index_query_context->candidate_rows != nullptr &&
1284-
expr_ctx->root()->node_type() == TExprNodeType::COMPOUND_PRED;
1293+
effective_root->node_type() == TExprNodeType::COMPOUND_PRED;
12851294
const roaring::Roaring* saved = suppress ? _index_query_context->candidate_rows : nullptr;
12861295
if (suppress) {
12871296
_index_query_context->candidate_rows = nullptr;

be/test/storage/index/inverted/query/phrase_prefix_query_test.cpp

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,13 @@
2323

2424
#include "io/fs/local_file_system.h"
2525
#include "runtime/exec_env.h"
26+
#include "storage/compaction/collection_similarity.h"
2627
#include "storage/index/index_file_reader.h"
2728
#include "storage/index/index_file_writer.h"
2829
#include "storage/index/inverted/inverted_index_cache.h"
2930
#include "storage/index/inverted/inverted_index_searcher.h"
3031
#include "storage/index/inverted/inverted_index_writer.h"
32+
#include "storage/index/inverted/similarity/collection_statistics.h"
3133
#include "storage/tablet/tablet_schema.h"
3234
#include "util/slice.h"
3335

@@ -311,6 +313,87 @@ TEST_F(PhrasePrefixQueryTest, test_multi_term_phrase_prefix_query) {
311313
EXPECT_GE(result.cardinality(), 0);
312314
}
313315

316+
TEST_F(PhrasePrefixQueryTest, scoring_uses_norm_capable_exact_term_iterator) {
317+
std::string_view rowset_id = "test_scoring_norm_source";
318+
int seg_id = 0;
319+
320+
// "common" has df=5 while the rare* expansion union has df=2. With a
321+
// one-row candidate the sorted DISI order is candidate, rare* union,
322+
// common exact term, so choosing the first non-candidate iterator as the
323+
// norm source selects a UnionTermIterator that cannot provide norms.
324+
std::vector<Slice> values = {Slice("common rareone"), Slice("common raretwo"),
325+
Slice("common filler"), Slice("common filler"),
326+
Slice("common filler")};
327+
328+
TabletIndex idx_meta;
329+
auto index_meta_pb = std::make_unique<TabletIndexPB>();
330+
index_meta_pb->set_index_type(IndexType::INVERTED);
331+
index_meta_pb->set_index_id(1);
332+
index_meta_pb->set_index_name("test_scoring_norm_source");
333+
index_meta_pb->clear_col_unique_id();
334+
index_meta_pb->add_col_unique_id(1);
335+
index_meta_pb->mutable_properties()->insert({"parser", "english"});
336+
index_meta_pb->mutable_properties()->insert({"lower_case", "true"});
337+
index_meta_pb->mutable_properties()->insert({"support_phrase", "true"});
338+
idx_meta.init_from_pb(*index_meta_pb);
339+
340+
std::string index_path_prefix;
341+
prepare_fulltext_index(rowset_id, seg_id, values, &idx_meta, &index_path_prefix);
342+
auto searcher = create_searcher(index_path_prefix, idx_meta);
343+
ASSERT_NE(searcher, nullptr);
344+
345+
class FixedStats : public CollectionStatistics {
346+
public:
347+
float get_or_calculate_idf(const std::wstring&, const std::wstring&) override {
348+
return 1.0F;
349+
}
350+
float get_or_calculate_avg_dl(const std::wstring&) override { return 2.0F; }
351+
};
352+
353+
RuntimeState runtime_state;
354+
TQueryOptions query_options;
355+
query_options.inverted_index_max_expansions = 50;
356+
runtime_state.set_query_options(query_options);
357+
io::IOContext io_ctx;
358+
roaring::Roaring candidate;
359+
candidate.add(0);
360+
361+
InvertedIndexQueryInfo query_info;
362+
query_info.field_name = L"1";
363+
query_info.term_infos.emplace_back("common", 0);
364+
query_info.term_infos.emplace_back("rare", 1);
365+
query_info.is_similarity_score = true;
366+
367+
auto run_scoring = [&](const roaring::Roaring* candidate_rows) {
368+
IndexQueryContextPtr context = std::make_shared<IndexQueryContext>();
369+
context->io_ctx = &io_ctx;
370+
context->runtime_state = &runtime_state;
371+
context->collection_statistics = std::make_shared<FixedStats>();
372+
context->collection_similarity = std::make_shared<CollectionSimilarity>();
373+
context->candidate_rows = candidate_rows;
374+
375+
PhrasePrefixQuery query(searcher, context);
376+
query.add(query_info);
377+
roaring::Roaring result;
378+
query.search(result);
379+
return std::pair {std::move(result), context->collection_similarity->release_scores()};
380+
};
381+
382+
// The low-df union is also first without a candidate. This pins down that
383+
// the scoring failure predates candidate pushdown rather than attributing
384+
// it to the optimization merely because both share the DISI ordering.
385+
auto [full_result, full_scores] = run_scoring(nullptr);
386+
EXPECT_EQ(full_result.cardinality(), 2);
387+
EXPECT_TRUE(full_result.contains(0));
388+
EXPECT_TRUE(full_result.contains(1));
389+
EXPECT_TRUE(full_scores.contains(0));
390+
EXPECT_TRUE(full_scores.contains(1));
391+
392+
auto [restricted_result, restricted_scores] = run_scoring(&candidate);
393+
EXPECT_EQ(restricted_result, candidate);
394+
EXPECT_TRUE(restricted_scores.contains(0));
395+
}
396+
314397
TEST_F(PhrasePrefixQueryTest, test_empty_terms_exception) {
315398
std::string_view rowset_id = "test_empty_terms";
316399
int seg_id = 0;
@@ -519,4 +602,4 @@ TEST_F(PhrasePrefixQueryTest, test_phrase_with_no_prefix_expansion) {
519602
EXPECT_GE(result.cardinality(), 0);
520603
}
521604

522-
} // namespace doris::segment_v2
605+
} // namespace doris::segment_v2

be/test/storage/segment/segment_iterator_candidate_pushdown_test.cpp

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,10 @@
3232
#include "common/config.h"
3333
#include "common/status.h"
3434
#include "core/data_type/data_type_number.h"
35+
#include "exprs/vcompound_pred.h"
3536
#include "exprs/vexpr.h"
3637
#include "exprs/vexpr_context.h"
38+
#include "exprs/virtual_slot_ref.h"
3739
#include "runtime/runtime_state.h"
3840
#include "storage/index/index_iterator.h"
3941
#include "storage/index/index_query_context.h"
@@ -96,6 +98,55 @@ class CapturingExpr : public VExpr {
9698
const roaring::Roaring* _captured_candidate = nullptr;
9799
};
98100

101+
// Produces a deterministic inverted-index result while modeling the contract
102+
// of a candidate-consuming leaf: only its TRUE bitmap is candidate-restricted;
103+
// its NULL bitmap remains segment-wide so compound SQL three-valued logic can
104+
// distinguish FALSE from UNKNOWN.
105+
class CandidateRestrictedBitmapExpr : public VExpr {
106+
public:
107+
CandidateRestrictedBitmapExpr(SegmentIterator* iter, std::initializer_list<uint32_t> true_rows,
108+
std::initializer_list<uint32_t> null_rows)
109+
: _iter(iter) {
110+
_data_type = make_nullable(std::make_shared<DataTypeUInt8>());
111+
for (uint32_t row : true_rows) {
112+
_true_rows.add(row);
113+
}
114+
for (uint32_t row : null_rows) {
115+
_null_rows.add(row);
116+
}
117+
}
118+
119+
const std::string& expr_name() const override {
120+
static const std::string kName = "CandidateRestrictedBitmapExpr";
121+
return kName;
122+
}
123+
124+
Status execute_column_impl(VExprContext*, const Block*, const Selector*, size_t,
125+
ColumnPtr&) const override {
126+
return Status::NotSupported("bitmap-only test expression");
127+
}
128+
129+
Status evaluate_inverted_index(VExprContext* context, uint32_t) override {
130+
_evaluated = true;
131+
auto data = std::make_shared<roaring::Roaring>(_true_rows);
132+
if (_iter->_index_query_context->candidate_rows != nullptr) {
133+
*data &= *_iter->_index_query_context->candidate_rows;
134+
}
135+
context->get_index_context()->set_index_result_for_expr(
136+
this, InvertedIndexResultBitmap(std::move(data),
137+
std::make_shared<roaring::Roaring>(_null_rows)));
138+
return Status::OK();
139+
}
140+
141+
bool evaluated() const { return _evaluated; }
142+
143+
private:
144+
SegmentIterator* _iter;
145+
roaring::Roaring _true_rows;
146+
roaring::Roaring _null_rows;
147+
bool _evaluated = false;
148+
};
149+
99150
// An indexed predicate stub that shrinks the row bitmap to a fixed set,
100151
// standing in for a selective indexed equality applied before the expression
101152
// conjuncts (modeled on MockNestedPredicate of accept_null_predicate_test).
@@ -167,6 +218,44 @@ VExprContextSPtr make_capturing_ctx(const std::shared_ptr<CapturingExpr>& expr)
167218
return ctx;
168219
}
169220

221+
TExprNode make_compound_node(TExprOpcode::type opcode, int num_children) {
222+
TExprNode node;
223+
node.__set_type(create_type_desc(PrimitiveType::TYPE_BOOLEAN));
224+
node.__set_node_type(TExprNodeType::COMPOUND_PRED);
225+
node.__set_opcode(opcode);
226+
node.__set_num_children(num_children);
227+
node.__set_is_nullable(true);
228+
return node;
229+
}
230+
231+
VExprContextSPtr make_virtual_slot_ctx(const VExprSPtr& virtual_expr) {
232+
TExprNode node;
233+
node.__set_type(create_type_desc(PrimitiveType::TYPE_BOOLEAN));
234+
node.__set_node_type(TExprNodeType::VIRTUAL_SLOT_REF);
235+
node.__set_num_children(0);
236+
node.__set_is_nullable(true);
237+
node.__set_label("virtual_compound");
238+
TSlotRef slot_ref;
239+
slot_ref.__set_slot_id(-1);
240+
slot_ref.__set_tuple_id(-1);
241+
node.__set_slot_ref(slot_ref);
242+
243+
auto root = VirtualSlotRef::create_shared(node);
244+
root->set_virtual_column_expr(virtual_expr);
245+
static const std::string kColumnName = "virtual_compound";
246+
root->set_column_name(&kColumnName);
247+
root->set_column_data_type(make_nullable(std::make_shared<DataTypeUInt8>()));
248+
249+
auto ctx = std::make_shared<VExprContext>(root);
250+
std::vector<std::unique_ptr<IndexIterator>> index_iters;
251+
std::vector<IndexFieldNameAndTypePair> storage_types;
252+
std::unordered_map<ColumnId, std::unordered_map<const VExpr*, bool>> status_map;
253+
ColumnIteratorOptions column_iter_opts;
254+
ctx->set_index_context(std::make_shared<IndexExecContext>(
255+
index_iters, storage_types, status_map, nullptr, nullptr, column_iter_opts));
256+
return ctx;
257+
}
258+
170259
} // namespace
171260

172261
class SegmentIteratorCandidatePushdownTest : public testing::Test {
@@ -280,6 +369,36 @@ TEST_F(SegmentIteratorCandidatePushdownTest, compound_root_evaluates_without_can
280369
EXPECT_EQ(_iter->_index_query_context->candidate_rows, nullptr);
281370
}
282371

372+
TEST_F(SegmentIteratorCandidatePushdownTest,
373+
virtual_slot_wrapped_compound_preserves_three_valued_logic) {
374+
config::inverted_index_candidate_pushdown_ratio = 0.3;
375+
_iter->_row_bitmap.add(0); // 1% of 100 rows: candidate engages
376+
377+
// At row 0, A is NULL and B is FALSE, so SQL requires
378+
// NOT(A AND B) = NOT(FALSE) = TRUE. A also has a TRUE row outside the
379+
// candidate so a candidate-restricted evaluation makes its TRUE bitmap
380+
// empty and exposes VCompoundPred's invalid early exit.
381+
auto nullable_a = std::make_shared<CandidateRestrictedBitmapExpr>(
382+
_iter.get(), std::initializer_list<uint32_t> {50}, std::initializer_list<uint32_t> {0});
383+
auto false_b = std::make_shared<CandidateRestrictedBitmapExpr>(
384+
_iter.get(), std::initializer_list<uint32_t> {}, std::initializer_list<uint32_t> {});
385+
auto and_expr = VCompoundPred::create_shared(make_compound_node(TExprOpcode::COMPOUND_AND, 2));
386+
and_expr->add_child(nullable_a);
387+
and_expr->add_child(false_b);
388+
auto not_expr = VCompoundPred::create_shared(make_compound_node(TExprOpcode::COMPOUND_NOT, 1));
389+
not_expr->add_child(and_expr);
390+
auto ctx = make_virtual_slot_ctx(not_expr);
391+
_iter->_common_expr_ctxs_push_down = {ctx};
392+
393+
ASSERT_TRUE(_iter->_get_row_ranges_by_column_conditions().ok());
394+
395+
const auto* result = ctx->get_index_context()->get_index_result_for_expr(not_expr.get());
396+
ASSERT_NE(result, nullptr);
397+
EXPECT_TRUE(false_b->evaluated()) << "AND must evaluate FALSE B after nullable A";
398+
EXPECT_TRUE(result->get_data_bitmap()->contains(0))
399+
<< "NOT(NULL AND FALSE) must preserve candidate row 0";
400+
}
401+
283402
// A non-finite configured ratio must never engage the pushdown (the multiply
284403
// and integer conversion would otherwise be undefined behavior).
285404
TEST_F(SegmentIteratorCandidatePushdownTest, non_finite_ratio_never_engages) {

0 commit comments

Comments
 (0)