github-actions[bot] commented on code in PR #66979:
URL: https://github.com/apache/doris/pull/66979#discussion_r3821643870
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -1786,11 +1786,17 @@ Status SegmentIterator::_init_index_iterators() {
_index_query_context->io_ctx = &_opts.io_ctx;
_index_query_context->stats = _opts.stats;
_index_query_context->runtime_state = _opts.runtime_state;
+ if (auto it = _opts.delete_bitmap.find(segment_id()); it !=
_opts.delete_bitmap.end()) {
+ _index_query_context->delete_bitmap = it->second;
+ }
if (_score_runtime) {
_index_query_context->collection_statistics =
_opts.collection_statistics;
_index_query_context->collection_similarity =
std::make_shared<CollectionSimilarity>();
- _index_query_context->query_limit = _score_runtime->get_limit();
+ _index_query_context->query_limit =
Review Comment:
[P1] Gate early TopK on the complete visible candidate domain
Absence of a rowset delete predicate is not enough to make segment-local
TopK safe. For example, with a higher-scoring `id=1` and lower-scoring `id=2`,
`WHERE SEARCH('title:alpha') AND id=2 ORDER BY score() DESC LIMIT 1` lets
`FunctionSearch` keep only id=1 from the full index, after which the existing
id/key predicate intersects it away and id=2 cannot be recovered. UNIQUE-MOR
has the same issue when an explicit `__DORIS_DELETE_SIGN__=1` row or version
merge removes a high-scoring key above `SegmentIterator`; neither source
appears in this predicate count or the MOW bitmap. Please enable early TopK
only when no later filter/merge can reject a hit, or feed the complete
admissible-row domain into the collector, and cover both triggers.
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -3806,7 +3812,8 @@ void
SegmentIterator::_prepare_score_column_materialization() {
IColumn::MutablePtr result_column;
auto result_row_ids = std::make_unique<std::vector<uint64_t>>();
- if (_score_runtime->get_limit() > 0 && _col_predicates.empty() &&
+ if (_opts.delete_condition_predicates->num_of_column_predicate() == 0 &&
Review Comment:
[P1] Avoid materializing every score for a small TopK query
Together with `query_limit = 0`, this branch makes any applicable delete
predicate turn `ORDER BY score() ... LIMIT 1` into full-match collection:
`collect_multi_segment_doc_set` inserts every score into `_bm25_scores`, then
`get_bm25_scores` allocates row-id and score arrays for the entire bitmap
before the delete predicate runs. A broad term on an old rowset with even one
delete predicate can therefore consume O(matches) memory/CPU instead of bounded
TopK resources. Please make the candidate collection
visibility-aware/refillable while remaining bounded, or add a concrete resource
guard with scale evidence that prevents this fallback from exhausting a query.
##########
be/test/storage/index/inverted/query_v2/multi_segment_collector_test.cpp:
##########
@@ -142,6 +143,41 @@ TEST_F(MultiSegmentCollectorTest,
CollectDocSetWithMultiReader) {
_CLDECDELETE(dir1);
}
+TEST_F(MultiSegmentCollectorTest, CollectTopKExcludesDeletedDocs) {
+ auto* dir0 = FSDirectory::getDirectory((kTestDir + "/segment0").c_str());
+ auto* dir1 = FSDirectory::getDirectory((kTestDir + "/segment1").c_str());
+
+ ValueArray<lucene::index::IndexReader*> readers(2);
+ readers[0] = lucene::index::IndexReader::open(dir0, true);
+ readers[1] = lucene::index::IndexReader::open(dir1, true);
+ auto reader = make_shared_reader(_CLNEW
lucene::index::MultiReader(&readers, true));
+
+ auto index_query_context = std::make_shared<IndexQueryContext>();
+ auto field = StringHelper::to_wstring("title");
+ TermQuery query(index_query_context, field,
StringHelper::to_wstring("fleabag"));
Review Comment:
[P2] Exercise the offset and actual block-WAND paths
This case does not fail if `+ seg_base` is removed: bitmap `{0}` still
deletes first-reader local doc 0, while the expected second-reader hit is local
doc 1/global doc 3 and remains admitted. Also, `TermQuery::weight(false)` is
held as `WeightPtr`; `TermWeight` has only a templated same-name overload, not
a virtual `PruningCallback` override, so the `use_wand=true` leg takes
`Weight`'s generic scorer loop just like `false`, and these two hits have equal
frequency/length rather than a deleted high scorer plus a lower replacement.
Please add a nonzero-base deletion that fails on local-id lookup, plus a
genuinely scoring Boolean query whose `OccurBooleanWeight` enters block WAND
with a deleted high scorer and a visible replacement.
##########
regression-test/suites/search/test_search_score_topn_delete_predicate.groovy:
##########
@@ -0,0 +1,111 @@
+// 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.
+
+suite("test_search_score_topn_delete_predicate", "p0") {
+ def insertRows = { tableName, rows ->
+ sql "INSERT INTO ${tableName} VALUES ${rows}"
+ sql "SYNC"
+ }
+
+ def assertScoreTopN = { tableName, predicate, expectedId ->
+ assertEquals(expectedId, sql("""
Review Comment:
[P2] Use the regression golden-output mechanism
These are deterministic query results, but the suite wraps them in raw
`assertEquals(sql(...))` calls and adds no generated `.out`. The repository
testing rules require `qt_`/`order_qt_` (or explicit `ORDER BY`) with
runner-generated expected output rather than Groovy assertions for determined
results. Please convert the cases to named query tests and commit the generated
output so the full rows/types are reviewed by the normal regression harness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
