Repository navigation
[fix](be) Keep BM25 statistics per index for MATCH on one column - #69901
Merged
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#69790 Problem Summary: score() over MATCH clauses that read different inverted indexes of one column fails: INDEX idx_english(name) USING INVERTED PROPERTIES("parser" = "english", ...), INDEX idx_standard(name) USING INVERTED PROPERTIES("parser" = "standard", ...) SELECT id, score() FROM t WHERE name MATCH 'apple' USING ANALYZER english AND name MATCH 'apple' USING ANALYZER standard ORDER BY score() DESC LIMIT 10; -- Scoring predicates for one field select different inverted indexes BM25 statistics are keyed by field (column unique id plus suffix path), but each index of a column holds its own terms and lengths: over Apple and apple, an index that folds case has df(apple) = 2 and one that does not has df(apple) = 1. One entry cannot serve both, so the collector refuses the query. Fix: a MATCH keeps its statistics under the field and the index id. CollectInfo records the field as indexed for opening the index, since the map key no longer is one. The readers bind their source to the real field and pass the statistics name to run_leaf, so the scorers and similarities need no change: they already look statistics up by the name they are given. Each clause now scores with the statistics of its own index, and the combined score is the sum of what each clause scores alone. SEARCH keeps its own statistics names and its rejection of two indexes on one field; only MATCH changes. A MATCH and a SEARCH on one index keep separate entries with the same values. ### Release note score() over MATCH clauses that use different inverted indexes of one column, selected with USING ANALYZER, no longer fails. ### Check List (For Author) - Test: Regression test / Unit Test / Manual test - Regression test: new test_match_score_multi_index on V2/V3/SNII, with two indexes that differ in case folding, so df(apple) is 3 in one and 1 in the other: both clauses score as they do alone, and identically on the three formats. 26 suites in inverted_index_p0 and search pass, including the multi-analyzer ones - Unit Test: the collector test that pinned the rejection now pins both entries; tests that read MATCH statistics use the new key - Behavior changed: Yes. The query above returns scores instead of an error. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
|
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. |
Contributor
TPC-H: Total hot run time: 28484 ms |
Contributor
TPC-DS: Total hot run time: 150956 ms |
Contributor
ClickBench: Total hot run time: 24.37 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Member
Author
|
/review |
airborne12
marked this pull request as ready for review
October 31, 2026 02:18
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed all 13 changed files at head 91eace3. One verified P2 finding is attached inline: MATCH's synthetic statistics key can collide with a raw SEARCH VARIANT field and reject a previously valid mixed scoring query. No existing inline findings were present; existing_blocking_comment_ids is empty.
Flow inspected: MATCH expression -> selected index -> field/index statistics -> physical CLucene or SNII source -> BM25 -> score collection. SEARCH retains its raw field identity and shares the statistics collection map.
Critical checkpoint conclusions:
- Goal: per-index terms and lengths support the intended multi-index MATCH behavior on ordinary fields. The mixed VARIANT namespace collision still needs correction.
- Scope/clarity: the producer/consumer identity change and its tests are focused; the physical field is explicitly retained.
- Concurrency: no new threads or locks. Shared tablet statistics retain their existing collection mutex, and derived scoring caches remain scanner-local. No new lock-order issue found.
- Lifecycle/initialization: schema and shared ownership retain generated index metadata and opened readers through synchronous scoring. No new dangling reference or cross-translation-unit initialization dependency found.
- Configuration: no configuration items added or changed.
- Compatibility: no on-disk, wire-format or libc dependency change. The legacy unscored CLucene overload remains available; both CLucene formats and SNII are updated.
- Thrift/protobuf: no definition or field-ID changes; release-branch ID comparison is inapplicable.
- Parallel paths: inspected MATCH/SEARCH, V2/V3/SNII, direct and inherited/field-pattern VARIANT indexes, term sets, phrases, prefixes, regexp, phrase-edge, keyword/normalizer and candidate-restricted paths. No additional verified issue.
- Conditions/errors: no new corrective branch; the key helper explains its Apple/apple example. Existing Status and exception boundaries propagate failure. Hidden-row norm callbacks use the synthetic name only as a cache label and compute through the physical source.
- Test coverage: inspected changed collector/SNII unit tests and the regression covering each analyzer alone, AND/OR sums, same-index reuse and phrase-plus-term across all three formats. A mixed colliding VARIANT-key regression is missing. Existing hidden/NULL/cache and prefix paths were also inspected.
- Test results: all 18 new regression result blocks agree with the source BM25 formula and score composition by arithmetic inspection. Test execution and result-generation provenance were not independently verified.
- Observability: existing errors and statistics diagnostics retain field/index context; no additional metric requirement identified for this local change.
- Transactions/persistence: no transaction or persistence behavior changed. Hidden rows still use the scan's read-version delete bitmap, and document counts are accumulated once per segment.
- Data writes/crashes: no write or asynchronous publication path changed. Existing ownership and failure cleanup remain intact; no additional leak or atomicity defect found.
- FE/BE variables: none added. Existing FE parsing/binding/serialization was traced to establish that the reported VARIANT path is reachable.
- Performance/memory: additional strings and entries scale with selected indexes/terms. No new large buffer, ownership leak or material algorithmic regression identified; MATCH/SEARCH can repeat statistics work for the same index under separate labels.
- Other/core invariants: committed-row visibility, version bounds, delete-bitmap alignment and nullable execution are unchanged by the identity change. No additional verified defect or unresolved candidate remains.
No additional user focus was supplied. Review converged after 2 rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS in round 2, all risk points have evidence-backed dispositions, and the final changed-file/anchor sweep is complete. This was static inspection only: no builds, tests, benchmarks or runtime reproduction were performed, and no repository source was modified.
Review converged after 2 round(s).
| // terms and lengths under the same field, so the field alone cannot key them: over Apple and | ||
| // apple, an index that folds case has df(apple) = 2 and one that does not has df(apple) = 1. | ||
| inline std::wstring match_statistics_field(const std::wstring& field, int64_t index_id) { | ||
| return field + L"@" + std::to_wstring(index_id); |
Contributor
There was a problem hiding this comment.
[P2] Keep MATCH keys disjoint from raw SEARCH fields
A synthetic MATCH key can also be a valid SEARCH field for a VARIANT subcolumn. For a scoring-capable parent index with ID 2001 and parent column UID 1, MATCH on v['a'] collects under 1.v.a@2001, exactly the raw key used by search('alpha', '{"default_field":"v.a@2001"}'). Both collectors write to the same map, but their inherited index suffixes differ (v%2Ea versus v%2Ea@2001), so validate_same_physical_index() rejects whichever predicate is collected second. This makes a previously valid mixed MATCH/SEARCH query fail when it requests score(). Use a tagged or structured identity that cannot collide with raw SEARCH fields, and cover a VARIANT key ending in @<actual-index-id>.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Open
6 of 10 tasks
csun5285
approved these changes
Nov 2, 2026
guptashivam04
pushed a commit
to guptashivam04/doris
that referenced
this pull request
Nov 2, 2026
… 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
yiguolei
pushed a commit
that referenced
this pull request
Nov 5, 2026
…ARCH #69790 (#70008) Backport of #69790 to branch-4.2. ### What problem does this PR solve? Issue Number: None Related PR: #69790, #69051 Problem Summary: score() of a MATCH on an untokenized (keyword) inverted index is always 0, while SEARCH on the same field scores BM25: INDEX idx_redirect (redirect) USING INVERTED -- one row ('BP') SELECT score() FROM t WHERE redirect MATCH 'BP'; -- 0 SELECT score() FROM t WHERE SEARCH('redirect:BP'); -- 0.2876821 Elasticsearch scores match, term and match_phrase on a keyword field with BM25 in query context, so the score is the idf of the whole value. #69051 (already on 4.2) made the engine score keyword postings but kept MATCH out. This makes a keyword index eligible for MATCH scoring and collects its whole value as one term, the one the reader looks up. It also carries the follow-up fixes from #69790's review: - A keyword MATCH_PHRASE drops its ` ~N` slop before collecting. - A MATCH on a non-string leaf such as `CAST(n AS STRING)` collects nothing. - A VARIANT path that a segment indexed as BKD, or does not hold, adds no statistics. This covers the BKD searcher coming from the searcher cache, and SNII. - A SNII index kind is looked up without copying the directory. ### Branch adaptations - `can_score_match` keeps 4.2's analyzed rule (support_phrase only); branch-4.2 has no lowercase-normalizer clause. - `test_omit_norms` does not exist on 4.2, so its `.out` change is dropped. The branch was cut before #69922 (the 4.2 backport of #69815) and then merged with branch-4.2 to resolve its conflict in `index_reader_helper_test.cpp`: both tests are kept, as on master. - `test_match_keyword_score` inserts its VARIANT rows as JSON strings: `parse_to_variant` in `VALUES` fails on 4.2 with a max-subcolumns conversion error. The expected output is master's file, unchanged. - **#69901 (BM25 statistics per index) is not on 4.2.** A MATCH on a keyword index and a MATCH on an analyzed index of the same column in one `score()` query (`name MATCH 'apple' USING ANALYZER english AND name MATCH 'apple' USING ANALYZER none`) now fails with "Scoring predicates for one field select different inverted indexes". Master behaved the same before #69901. Before this change, that query ran with the keyword clause scoring 0. Each clause alone scores fine. Two analyzed indexes on one column already fail this way on 4.2. Backporting #69901 lifts this. ### Validation On this branch merged with branch-4.2 c2aaed9, ASAN build: - BE UT: 1611 tests in `IndexReaderHelperTest`, `CollectionStatistics*` and `*Snii*` pass. - Regression: `test_match_keyword_score` passes against master's `.out` on V2/V3/SNII. The following also pass: `test_match_phrase_prefix_score`, `test_bm25_score`, `test_score_filter_matrix`, `test_score_filter_usage`, `test_search_score_topn_predicates`, `test_multi_analyzer_query`, `test_match_using_analyzer`, `test_single_column_multi_index` (with debug points on), `test_search_score_keyword_filters`, `test_search_score_raw_index`, `test_search_score_pattern_constant`. - Local note: the local thirdparty predates the lance-c upgrade, so `lance_reader.cpp`'s `lance_scanner_set_scalar_index_auto_selection` call was stubbed for the local build only. It is not part of this PR. ### Release note score() of MATCH / MATCH_ALL / MATCH_PHRASE on a keyword (untokenized) inverted index now returns the BM25 score of the value, the same as SEARCH, instead of 0. ### Check List (For Author) - Test: Regression test / Unit Test (see Validation) - Behavior changed: Yes. Keyword MATCH contributes its BM25 score to score() instead of 0. - Does this need documentation: No 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #69790
Problem Summary: score() over MATCH clauses that read different inverted
indexes of one column fails:
BM25 statistics are keyed by field (column unique id plus suffix path),
but each index of a column holds its own terms and lengths: over Apple
and apple, an index that folds case has df(apple) = 2 and one that does
not has df(apple) = 1. One entry cannot serve both, so the collector
refuses the query.
Fix: a MATCH keeps its statistics under the field and the index id.
CollectInfo records the field as indexed for opening the index, since the
map key no longer is one. The readers bind their source to the real field
and pass the statistics name to run_leaf, so the scorers and similarities
need no change: they already look statistics up by the name they are
given. Each clause now scores with the statistics of its own index, and
the combined score is the sum of what each clause scores alone.
SEARCH keeps its own statistics names and its rejection of two indexes on
one field; only MATCH changes. A MATCH and a SEARCH on one index keep
separate entries with the same values.
Release note
score() over MATCH clauses that use different inverted indexes of one
column, selected with USING ANALYZER, no longer fails.
Check List (For Author)
with two indexes that differ in case folding, so df(apple) is 3 in
one and 1 in the other: both clauses score as they do alone, and
identically on the three formats. 26 suites in inverted_index_p0
and search pass, including the multi-analyzer ones
both entries; tests that read MATCH statistics use the new key
error.
🤖 Generated with Claude Code