Repository navigation
[fix](inverted index) Count delete-sign rows in BM25 statistics when the scan returns them - #69854
guptashivam04 wants to merge 9 commits into
Conversation
…the scan returns them With skip_delete_sign or show_hidden_columns a query on a merge-on-write table returns the rows written with the delete sign and scores them, but the statistics still left those rows out, so their scores differed from those of a table holding the rows the query returns. The FE filters these rows by adding __DORIS_DELETE_SIGN__ = 0 to the scan and leaves the conjunct out for both settings, which it does not send to the BE. The scan now notes whether a conjunct reads the delete-sign slot before the conjuncts are normalized, and the statistics hide rows with the delete sign only when it does. Rows the delete bitmap marks stay hidden whatever the query sets.
…istics cached Merge-on-write statistics on SNII opened each segment's logical index and summed its norms again on every query. They now take the logical index from the searcher cache, as the query path does, and keep the norm length sums with it, as CLucene keeps them with its searcher. The sums move to a NormLengthSums that both cache values hold, so they are bounded by and evicted with the searcher cache. The sum of a set of hidden rows is now keyed by the hidden rows themselves, so a query whose sums are kept reads neither norms nor NULL rows on either format. Before, every CLucene query filtered its hidden rows by the null bitmap to build the key. Norms of selected rows are decoded 4096 rows at a time instead of into a buffer as large as the selection, which the memory tracker did not see.
… context The statistics read a CLucene field's norms through the stream the reader keeps for them, which every query of a cached searcher shares and which carries no query's context, so the reads ignored disable_file_cache and the remote cache write limiter and counted to no query. DorisCompoundReader::ScopedIOContext gives the reads a thread makes through sub-file streams without a context of their own the query's context for the duration of the norm read. CSIndexInput drops the context from the shared base stream after every read, also when the read throws, so no query's pointer stays behind on a cached reader.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 28570 ms |
TPC-DS: Total hot run time: 151375 ms |
ClickBench: Total hot run time: 24.15 s |
|
/review |
There was a problem hiding this comment.
Static review of PR 69854 was completed against the exact head 2934eca and converged after 2 bounded rounds. The change has two verified findings: explicit predicates that reference DORIS_DELETE_SIGN can make BM25 statistics hide rows the predicate returns, and the new NormLengthSums cache repeats full norm/NULL scans for concurrent cold misses. The delete-sign path was traced from FE filter injection through scan normalization, parallel scanners, shared tablet statistics, and collection-statistics consumption. The norm/cache and scoped-I/O paths were checked for cache identity, reader lifetime, lock ordering, exception cleanup, batching, NULL handling, and SNII/CLucene parity. No existing inline threads were present, so existing_blocking_comment_ids is empty. There was no additional user-provided focus. Changed tests and regression coverage were inspected, but no builds or tests were run because this invocation is static-only. Non-blocking coverage gaps remain for the production SNII cache branch and multi-field SNII fixture; static inspection found no additional actionable defect.
Review converged after 2 round(s).
| const int slot_id = (*delete_sign)->id(); | ||
| auto reads_delete_sign = [slot_id](const VExprContextSPtr& conjunct) { | ||
| return conjunct->root() != nullptr && expr_reads_slot(conjunct->root(), slot_id); | ||
| }; |
There was a problem hiding this comment.
[P1] Distinguish the default delete-sign filter from user predicates. With show_hidden_columns=true, WHERE __DORIS_DELETE_SIGN__ = 1 reaches this scan conjunct, so _conjuncts_read_delete_sign() returns true and collect_hidden_docs() removes all delete-sign rows from BM25 doc/token statistics even though those rows pass the predicate and are scored. This changes avgdl/idf and can change ORDER BY score()/LIMIT results. Detect the FE-generated = 0 filter or carry its semantics explicitly, and add coverage for nonzero predicates.
There was a problem hiding this comment.
Fixed in 2f7b088. Only a scan conjunct that is exactly __DORIS_DELETE_SIGN__ = 0 (the FE's filter, or the same condition written by the query) now hides the delete-sign rows from the statistics. Any other predicate on the column, such as = 1, != 0 or >= 0, leaves them counted, like a predicate on any other column.
Covered by OlapScanOperatorDeleteSignTest.OtherPredicateOnDeleteSignDoesNotFilterIt and a new show_hidden_columns + __DORIS_DELETE_SIGN__ = 1 case in test_bm25_score_delete_sign_statistics on V2/V3/SNII, which fails without the change (80 rows score differently).
| } | ||
| // Computed outside the lock, which other queries of the index take to read their sums. | ||
| // Two queries may both compute a sum the index does not hold yet; both get the same. | ||
| RETURN_IF_ERROR(compute(out)); |
There was a problem hiding this comment.
[P2] Avoid duplicate first-use norm scans. This path unlocks _mutex before compute(out), and the hidden-row method does the same, so concurrent BM25 queries can all miss the same field/set and each run source.sum_norm_lengths() plus NULL-row decoding before racing to cache the result. Large segments and query bursts therefore multiply full norm I/O, CPU, and temporary buffers. Use per-key single-flight state or serialize the first computation, and add a concurrent first-use test.
There was a problem hiding this comment.
Fixed in ea8bcaf. The first query that needs a sum computes it. Queries that need the same sum meanwhile wait for it instead of reading the norms, while other fields and other hidden-row sets are not blocked. If the computation fails or throws, the key is released and a waiting query computes the sum itself.
Covered by NormLengthSumsTest.ConcurrentQueriesComputeASumOnce (50 repeated runs on ASAN). Also checked on a 3-BE cluster: 16 clients starting together on cold caches, and a cancel/timeout stress run with cache evictions. No stuck queries, and every row keeps one score.
| std::move(opened), reader_size, UnixMillis(), file_reader); | ||
| InvertedIndexSearcherCache::instance()->insert(cache_key, cache_value, &out->cache_handle); | ||
| } | ||
| auto* cache_value = out->cache_handle.get<segment_v2::SniiLogicalReaderCacheValue>(); |
There was a problem hiding this comment.
[P1] A cached SNII BKD searcher reaches this static_cast and crashes the BE, on every SNII table.
The BKD reader caches SniiBkdSearcherCacheValue under the same key. After CAST(v['n'] AS BIGINT) = 1 on a VARIANT path indexed as BKD, a scoring CAST(v['n'] AS STRING) MATCH '1' hits that entry here. Before this PR the same query only failed with logical index is not a text inverted index.
Reproduced on ASAN: UBSan reports object is of type SniiBkdSearcherCacheValue, then ASan reports a heap-buffer-overflow in SniiIndexSource::scoring_stats, on MoW and DUPLICATE tables. This was with #69790, #69901 and this PR merged and the #69790 check removed; I did not build 2934eca alone. Unit tests cannot catch it, because they take the BE_TEST branch.
Depends on: merge #69790 (adds snii_field_has_no_terms, which skips BKD fields) and #69901 (changes the MATCH statistics key) first.
After they merge, this PR needs to:
- Resolve the 2 conflicts in
collection_statistics.cpp, keepingsnii_field_has_no_termsbeforeopen_snii_stats_reader. - Change
DeleteSignRowsCountWhenTheScanReturnsThemto readkMatchFieldinstead ofL"1". Git merges this file cleanly, but the test fails withNot such column 1.
With both done, 32 score() suites and 1629 BE unit tests pass on ASAN. Reference: https://github.com/airborne12/apache-doris/tree/verify-bkd-69854
There was a problem hiding this comment.
Merged master with #69790: snii_field_has_no_terms now runs before open_snii_stats_reader, so a field indexed as BKD is skipped before the searcher cache is looked up. With your sequence (CAST(v['n'] AS BIGINT) = 1, then CAST(v['n'] AS STRING) MATCH '1' with score()) on SNII MoW and duplicate tables, the release build stays up and those rows score 0. All 32 score() suites and 261 unit tests pass after the merge.
|
Thanks @airborne12 for reproducing the BKD crash and for the reference branch. I'll follow your plan: once #69790 and #69901 merge, I'll merge master into this branch, keep Meanwhile I'm working on the two bot findings (user predicates on |
… for the delete-sign filter ### What problem does this PR solve? Issue Number: None Related PR: apache#69438 Problem Summary: The BM25 statistics of a merge-on-write table leave out the rows written with the delete sign when the scan filters them. The scan told that by any conjunct that reads `__DORIS_DELETE_SIGN__`. With `show_hidden_columns = true`, a query's own predicate on the column, such as `WHERE __DORIS_DELETE_SIGN__ = 1`, then counted as the filter: the query returned and scored only the delete-sign rows, while the statistics left exactly those rows out, so their scores differed from the scores of the same rows without the predicate. The scan now treats as the filter only a conjunct that is `__DORIS_DELETE_SIGN__ = 0`, the one the FE adds unless the query sets `skip_delete_sign` or `show_hidden_columns`. Any other predicate on the column is a predicate of the query, which the statistics follow no more than a predicate on another column, so the delete-sign rows stay counted. A query that writes `= 0` itself keeps exactly the rows the FE's conjunct keeps and scores as the default. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test - OlapScanOperatorDeleteSignTest.OtherPredicateOnDeleteSignDoesNotFilterIt: `= 1`, `!= 0` and `>= 0` on the delete sign do not filter it, `= 0` does. - test_bm25_score_delete_sign_statistics: with `show_hidden_columns = true` and `__DORIS_DELETE_SIGN__ = 1`, every returned row scores as in a duplicate-key table loaded with the rows the query returns without the predicate, on V2, V3 and SNII. Without this change the case fails on V2 at once (80 rows score differently); the .out was regenerated with -forceGenOut and adds only the `__DORIS_DELETE_SIGN__ = 1` block, whose scores equal those of the same rows under `show_hidden_columns` alone. - All 29 regression suites that use score() pass on a 3-BE cluster (release build), and the BM25, statistics, scanner and scan-operator unit tests pass on ASAN (257 tests). - Behavior changed: Yes. With `show_hidden_columns = true`, a query that filters `__DORIS_DELETE_SIGN__` other than by `= 0` scores on statistics that count the delete-sign rows. - Does this need documentation: No
…m once for concurrent queries ### What problem does this PR solve? Issue Number: None Related PR: apache#69438 Problem Summary: The norm length sums that BM25 statistics of merge-on-write tables keep with a cached index reader were computed outside the lock, so queries that missed the same sum at the same time each read the norms (and the NULL rows) to compute it. That happens after every load: the load hides older rows, so queries at the new version need a new hidden-row sum for every segment it touched, and a burst of concurrent queries read the same norms many times. Holding the lock while computing, as the CLucene sums did before, would instead make queries of other fields and other hidden rows of the index wait. A sum is now computed by the first query that needs it. A query that needs a sum being computed waits for it and reads the kept sum; queries of other fields or other sets of hidden rows go on. If computing a sum fails or throws, the key is given up and a waiting query computes it itself, so one query's failure never fails another. ### Release note None ### Check List (For Author) - Test: Unit Test - NormLengthSumsTest.ConcurrentQueriesComputeASumOnce: while one query computes a sum, four queries of the same sum wait and get it without computing, a query of another set of rows is not blocked, and after a failed computation a waiting query computes the sum. It passes 50 repeated runs on ASAN, with the other 256 BM25, statistics and scanner unit tests. - Manual test on a 3-BE cluster (release build): 16 clients starting together on cold caches, 5 rounds, on V3 and SNII tables with overwritten and deleted rows: every row has one score and paging with a tiebreaker repeats or misses no row. 90 s of 16 clients with cancelled and timed-out queries and cache evictions: no other errors, no stuck queries, no BE failure. - Behavior changed: No - Does this need documentation: No
… MATCH statistics key ### What problem does this PR solve? Issue Number: None Related PR: apache#69901 Problem Summary: apache#69901 keeps the BM25 statistics of a MATCH under the field and the index id (`match_statistics_field`), so CollectionStatisticsTest.DeleteSignRowsCountWhenTheScanReturnsThem, which collects them for a MATCH and still read them under the field alone, failed on master with "Not such column 1". The test now reads them under `kMatchField`, as the other MATCH tests of the file do. ### Release note None ### Check List (For Author) - Test: Unit Test - The test fails on master without this change ("Not such column 1") and passes with it; the BM25, statistics, scanner and scan-operator unit tests pass on ASAN (258 tests). - Behavior changed: No - Does this need documentation: No
|
run buildall |
TPC-H: Total hot run time: 28314 ms |
TPC-DS: Total hot run time: 151563 ms |
ClickBench: Total hot run time: 24.36 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…-followups # Conflicts: # be/src/storage/index/inverted/reader/collection_statistics.cpp
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 28968 ms |
TPC-DS: Total hot run time: 151611 ms |
ClickBench: Total hot run time: 24.4 s |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
…-followups # Conflicts: # be/test/exec/operator/olap_scan_operator_test.cpp
81d5a43
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all 19 changed files at 81d5a43 using static source inspection, two complete-review scopes and an independent risk review. One new P2 finding remains: statistics-created SNII cache entries omit hot-term lookup-cache initialization. No new P0/P1 finding was verified.
Critical checkpoint conclusions:
- Goal and evidence: the delete-sign policy now follows whether the scan has the FE's zero filter, while overwritten rows remain excluded by the versioned delete bitmap. Unit and V2/V3/SNII regression coverage was inspected; no runtime pass is claimed.
- Scope and clarity: the scan flag, shared norm sums, bounded decoding and scoped I/O context address the stated correctness and statistics-cost work. The SNII initialization omission is the inline finding.
- Concurrency: scanner threads share pinned readers; norm-sum maps/in-flight keys use one mutex, computation runs outside it and waiting releases it. Existing lazy reader state is separately synchronized. No new lock-order defect was substantiated. The old duplicate-first-use concern is fixed.
- Lifecycle and initialization: cache handles pin readers and their owning IndexFileReader; readers are destroyed before that owner. Query callbacks are synchronous. The new thread-local context pointer has constant initialization, restores its predecessor and requires no TLS destructor. Shared base context is reset before unlocking, including thrown reads.
- Configuration: no configuration item is added. Existing skip_delete_sign/show_hidden_columns choices are inferred from the actual scan conjunct; nonzero user predicates leave those rows counted.
- Compatibility: no on-disk format or wire contract changes. Source review found no new libc API dependency; no release ELF/glibc audit was performed under this static-only contract.
- Thrift/protobuf IDs: gensrc is untouched; cross-branch field-ID comparison is inapplicable.
- Parallel paths: CLucene V2/V3 and SNII use the shared sum abstraction; cloud/local scanner flow and physical segment identity remain intact. The SNII query/statistics initialization mismatch is reported inline.
- Special conditions: the exact eq/delete-slot/zero check matches FE predicate generation before normalization; comments explain absent/nonzero predicates and norm-less indexes. BKD/missing VARIANT paths are skipped before the typed SNII cache lookup.
- Test coverage: inspected scan predicate tests, NULL/hidden-row statistics, cached sums, per-key concurrent first computation and failed-compute retry, 4096-row norm batches, and explicit/scoped I/O precedence plus throwing-read cleanup. The production statistics-cache branch is excluded by BE_TEST, leaving the reported initialization path uncovered there.
- Expected results: the regression output's default/explicit-zero and skip/show pairs agree; deleted rows retain the corresponding returned-row scores. Fixture relationships are internally consistent. Actual numeric BM25 results and generated-output provenance were not independently reproduced.
- Observability: existing StatisticsCollectTime, file-cache counters and statistics/error logs cover the changed paths; no additional logging/metric defect was found.
- Transactions and persistence: no transaction, version publication, EditLog or replay logic changes. The read version still selects the delete bitmap.
- Data writes and atomicity: production changes affect read-side statistics/cache state. No write atomicity/failover change is introduced; rowset ownership remains pinned while used.
- FE/BE state transport: no new transmitted variable; the new flag is derived locally from existing expressions and passed to all statistics collection paths.
- Performance and memory: generic selected norms decode in bounded batches; large row-ID temporaries use tracked vectors, and four hidden sums are retained per field. CLucene overrides both sum methods, avoiding repeated whole-field reads from generic batching. SNII loses its existing nonresident hot-term lookup reuse when statistics populate a cold cache, as reported. No performance measurements were run.
- Other concerns and prior review: P1 threads 4430597044 (user delete-sign predicates) and 4430796090 (SNII BKD cache type) are fixed on this head; P2 thread 4430597050 (duplicate first-use work) is also fixed. None is reposted or retained as an existing blocking finding. No additional user focus was supplied.
Validation performed: build-support/check-build-hygiene.sh passed all pure-text checks. No builds, unit/regression tests, benchmarks or cluster execution were performed. No repository source was modified. Remaining limits are dynamic validation, binary ABI validation, numeric-output reproduction and the PR's pre-existing declared light-delete/row-TTL/cache-policy limits; there are no unresolved source-level candidates or uncovered changed files.
Review converged after 2 rounds: all three latest reviewers returned NO_NEW_VALUABLE_FINDINGS, all 19 changed files were covered, and every candidate/risk was accepted, deduplicated or dismissed with evidence.
Review converged after 2 round(s).
| InvertedIndexSearcherCache::CacheKey cache_key(file_reader->get_index_file_cache_key(index)); | ||
| if (!InvertedIndexSearcherCache::instance()->lookup(cache_key, &out->cache_handle)) { | ||
| auto opened = DORIS_TRY(open_snii_stats_reader(*file_reader, index, io_ctx)); | ||
| const size_t reader_size = std::max<size_t>(opened->memory_usage(), 1); |
There was a problem hiding this comment.
[P2] Enable hot-term caching before publishing the SNII reader
On a cold searcher cache, statistics populate this entry during OlapScanner::_prepare_impl, before the normal query reader opens. Unlike SniiIndexReader::_get_logical_reader, this path never calls enable_term_lookup_cache(), and later searcher-cache hits return without enabling it. For a nonresident dictionary, term_lookups_ therefore stays null for the reader's cached lifetime, so repeated hot-term queries keep reading and decoding their DICT blocks, including following non-scoring queries. Enable the lookup cache before memory_usage() and insertion here too, so its full reserved charge is included.
TPC-H: Total hot run time: 28646 ms |
TPC-DS: Total hot run time: 153735 ms |
ClickBench: Total hot run time: 24.03 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
Do not need deal with skip_delete_sign = true, this is just for debug. |
|
run buildall |
What problem does this PR solve?
Issue Number: None
Related PR: #69438 (BM25 statistics of merge-on-write tables count only the rows the scan returns). This PR carries the review points left open when #69438 was merged.
Problem Summary:
#69438 leaves out of a merge-on-write table's BM25 statistics the rows the delete bitmap hides, and the rows written with the delete sign, so that every replica of a tablet scores alike. Review raised three points it did not address before merging:
skip_delete_sign = trueorshow_hidden_columns = true, the FE leaves out the__DORIS_DELETE_SIGN__ = 0conjunct, so the scan returns and scores those rows, but the statistics still hide them. Their scores then differ from those of a table holding the rows the query returns. Reproduction: the new suite below fails on master at once (V2, skip_delete_sign, compared with the returned rows: 313 rows score differently).disable_file_cacheand the remote cache write limiter, and counted to no query's file cache statistics. Setting the query's context on that shared stream would leave a pointer to the query on a cached reader.Fix:
OlapScanLocalStatenotes, before the conjuncts are normalized into column predicates and storage expressions, whether a scan conjunct is__DORIS_DELETE_SIGN__ = 0, the one the FE adds (MowHiddenRows::filters_delete_sign). The statistics hide rows with the delete sign only when it is there. Rows the delete bitmap marks stay hidden whatever the query sets. Any other predicate of the query on the column, such asshow_hidden_columns = truewithWHERE __DORIS_DELETE_SIGN__ = 1, is a predicate like any other, which the statistics do not follow, so the delete-sign rows stay counted; a query that writes= 0itself keeps exactly the rows the FE's conjunct keeps and scores as the default.CluceneSearcherCacheValueinto aNormLengthSumsthat bothCluceneSearcherCacheValueandSniiLogicalReaderCacheValuehold: the sum of a field's non-NULL norm lengths, and the sum of the non-NULL hidden rows' lengths for the last four sets of hidden rows of a field. A set is keyed by the hidden rows themselves (count, first, last and an xxHash64 of the row IDs), so a query whose sums are kept reads neither norms nor NULL rows. A sum is computed once, by the first query that needs it: queries that need the same sum meanwhile wait for it instead of reading the same norms, while queries of other fields or other hidden rows go on, and if computing it fails a waiting query computes it. The sums take a few dozen bytes per field and are evicted with the reader, so the searcher cache's capacity bounds them.SniiIndexReader::_get_logical_reader), and insert it on a miss, instead of opening it on every query. A BKD index of the same segment caches its searcher under the same key, as a VARIANT path holding numbers does (CAST(v['n'] AS BIGINT) = 1), so the statistics first skip fields without terms (snii_field_has_no_terms, from [fix](be) Score MATCH on keyword inverted indexes like SEARCH #69790) and never look such an entry up.IndexSource::sum_norm_lengths_ofdecodes the norms of selected rows 4,096 rows at a time. The NULL row ID list SNII builds is charged to the query's memory.DorisCompoundReader::ScopedIOContextgives the reads a thread makes through sub-file streams without a context of their own the query's context, for the duration of the statistics' norm read.CSIndexInputdrops the context from the shared base stream after every read, also when the read throws, so no query's pointer stays on a cached reader.Not covered:
enable_mow_light_delete) are still counted, as the delete bitmap does not mark them. Left for a separate PR.mem_limit, where a 1 M-row SNII segment's reader takes about 1.4 MB) leaves the cache at once, for the query path as for the statistics. Such a segment's norms are still summed on every query, as before this PR.SegmentTermDocsthroughIndexReader::norms(field)) without a per-read I/O context, as before. This PR scopes the context only around the statistics' norm reads.enable_inverted_index_searcher_cache = false, which the query path honours.Release note
Fix BM25
score()on merge-on-write tables withskip_delete_signorshow_hidden_columns: the statistics now count the rows written with the delete sign that the query returns.Check List (For Author)
Test (release build, macOS arm64, 3 BEs on one host,
enable_sql_cache = false. Run on masterd420f9c41bwith these commits, and again on this branch after merging master378404eeb1, which includes [fix](be) Keep BM25 statistics per index for MATCH on one column #69901, and master952ed6904f, which includes [fix](be) Score MATCH on keyword inverted indexes like SEARCH #69790, with the same results. The branch also merges cleanly with [fix](inverted index) Do not block scanner threads while BM25 statistics are collected #69810.)Regression test:
inverted_index_p0/test_bm25_score_delete_sign_statistics(new). OnV2,V3andSNII, a merge-on-write table with overwritten rows, delete-sign rows (CDC-style inserts with__DORIS_DELETE_SIGN__ = 1) and aDELETE. Withskip_delete_sign = trueand withshow_hidden_columns = true, every returned row must score exactly as in a duplicate-key table loaded with the rows the query returns; by default, as in one loaded with the visible rows..outresults pin the scores of each setting, ashow_hidden_columnsquery that filters__DORIS_DELETE_SIGN__ = 0itself scores as the default, and one that filters__DORIS_DELETE_SIGN__ = 1scores its rows as without the predicate. It fails without this PR and passes with it.Existing suites: all 28 other suites that use
score()(ininverted_index_p0,search,ann_index_p0,variant_p0,nereids_rules_p0andfault_injection_p0, includingtest_bm25_score_replica_statisticsandtest_bm25_score_statistics_searcher_io_contextfrom [fix](inverted index) Count only visible rows in BM25 statistics of merge-on-write tables #69438 andtest_match_phrase_prefix_scorefrom [fix](inverted index) Score MATCH_PHRASE_PREFIX on the tablet idf of the terms the prefix expands to #69698) pass on a 3-BE cluster: 29 suites, 0 failed; after merging master378404eeb1, 31 suites (with [fix](be) Keep BM25 statistics per index for MATCH on one column #69901'stest_match_score_multi_indexand [fix](be) Score regexp and wildcard patterns with a constant like Elasticsearch #69815's pattern scoring suite), 0 failed; after merging master952ed6904f, 32 suites (with [fix](be) Score MATCH on keyword inverted indexes like SEARCH #69790'stest_match_keyword_score), 0 failed.Unit Test:
OlapScanOperatorDeleteSignTest.*(__DORIS_DELETE_SIGN__ = 0, also once pushed down as a storage expression, filters the rows;= 1,!= 0,>= 0, a conjunct on another column, an output-only delete-sign slot, or a table without one does not),NormLengthSumsTest.ConcurrentQueriesComputeASumOnce(while one query computes a sum, queries of the same sum wait and do not compute it, a query of other rows is not blocked, and after a failed computation a waiting query computes it; 50 repeated runs),CollectionStatisticsTest.DeleteSignRowsCountWhenTheScanReturnsThem(real CLucene segment: withfilters_delete_sign = falseonly the delete-bitmap rows are left out, without reading the delete-sign column),CollectionStatisticsHiddenRowsTest.ExcludeHiddenDocsKeepsSumsWithTheIndex(a repeated query with the same hidden rows reads neither norms nor NULL rows; other hidden rows read the NULL rows once and only their own norms),CollectionStatisticsHiddenRowsTest.SumNormLengthsOfDecodesBoundedBatches(10,000 selected rows decode in 4,096-row batches),NormLengthSumsTest.*, andDorisCompoundReaderTest.ScopedIoContextReachesStreamsWithoutTheirOwn(a sub-file stream without a context reads with the scoped one and counts to its statistics, a stream with its own keeps it, and the shared base stream holds no context after a read, also after a failing read). Run locally on macOS (ASAN): 257 tests in the BM25, collection-statistics, scanner, scan-operator, compound-reader, searcher-cache, inverted-index-reader, null-bitmap and null-predicate suites pass on masterd420f9c41bwith these commits, 258 after merging master378404eeb1, and 261 after merging master952ed6904f(with [fix](be) Score MATCH on keyword inverted indexes like SEARCH #69790's newCollectionStatisticsTestcases). Since [fix](be) Keep BM25 statistics per index for MATCH on one column #69901 keeps MATCH statistics under the field and the index id,DeleteSignRowsCountWhenTheScanReturnsThemreads them underkMatchField.Manual test.
Replicas: a table like the reproduction of [fix](inverted index) Count only visible rows in BM25 statistics of merge-on-write tables #69438 (4 indexed text fields, 3 replicas, 8 buckets, 8 loads, 4 rounds of CDC upserts, delete-sign deletes and a
DELETE), on V3 and on SNII, queried withSEARCH('note_search:jobs OR task_search:jobs OR meeting_search:jobs OR profile_search:jobs') AND accountid = 12345. Before any compaction and after a full compaction of one replica of every tablet (1 rowset on that replica, 15 on the others), every one of the 1,595 rows has one score across 15 random-replica runs anduse_fix_replica = 0/1/2, the scores after compaction equal those before, and each replica's scores equal those of a table freshly loaded with the visible rows (0 rows differ). Withskip_delete_sign = truethe two uncompacted replicas score identically; the compacted one returns 109 rows fewer, as base compaction has removed the delete-sign rows.BKD searcher in the cache: on SNII MoW and duplicate tables with 3 replicas, a VARIANT column with an inverted index and
nholding numbers,CAST(v['n'] AS BIGINT) = 1followed byCAST(v['n'] AS STRING) MATCH '1' ORDER BY score(), three times each, on the release build after merging [fix](be) Score MATCH on keyword inverted indexes like SEARCH #69790: no BE failure, and the matching rows score 0, as [fix](be) Score MATCH on keyword inverted indexes like SEARCH #69790 intends for a field without terms.Cost, against master
d420f9c41b, single-replica tables in one bucket,inverted_index_searcher_cache_limit = 1 GB. QuerySEARCH('content:jobs OR content:try') ORDER BY score() DESC LIMIT 100;StatisticsCollectTimeand query total from the profile, two alternating rounds each:With the default searcher cache of a BE with a 2 GB
mem_limit, 1 M-row SNII segments' readers do not stay in the cache, and their statistics cost about the same as before (44 ms vs 47 ms, measured on [fix](inverted index) Count only visible rows in BM25 statistics of merge-on-write tables #69438's head).Concurrency: 16 clients starting together on cold caches (every BE cache cleared before each of 5 rounds), each running the query above in full and paging through it 50 rows at a time with
ORDER BY score() DESC, job_id, on the V3 and SNII tables: every row has one score, and no paging repeats or misses a row. 90 s of 16 clients that cancel queries, time them out and evict the caches: no other errors, no stuck query, no BE failure, and the same check passes afterwards.Memory: BE tcmalloc allocated bytes (sum of the 3 BEs) before and after 1 and 4 queries on each table move within the ±150 MB the sum fluctuates by, with and without this PR, and grow no more with it.
Behavior changed:
skip_delete_sign = trueorshow_hidden_columns = true(without their own filter on__DORIS_DELETE_SIGN__) now count the delete-sign rows they return in the BM25 statistics, so their scores change. Default queries score as before.Does this need documentation?
Check List (For Reviewer who merge this PR)