Skip to content

[fix](fe) Reject score on non-scoring SNII indexes - #66872

Closed
airborne12 wants to merge 1 commit into
apache:masterfrom
airborne12:fix-snii-scoring-capability
Closed

[fix](fe) Reject score on non-scoring SNII indexes#66872
airborne12 wants to merge 1 commit into
apache:masterfrom
airborne12:fix-snii-scoring-capability

Conversation

@airborne12

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: None

Related PR: #66052

Problem Summary: FE admitted score() plans for every phrase-enabled SNII inverted index, but built-in analyzers and ordinary custom analyzers write only the docs-and-positions tier. They do not persist norms or semantic scoring metadata, so the BE later failed while opening scoring statistics. This PR rejects those plans at the selected-index admission gate and admits SNII scoring only for indexes created with a CommonGrams analyzer, which writes the scoring tier. Existing V3 behavior is unchanged. The change is query admission only and does not modify index writes or the storage format.

Release note

Reject score() queries on SNII indexes that do not persist scoring data instead of failing later in the BE.

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason

    Added end-to-end score pushdown cases for built-in, plain custom, and CommonGrams SNII analyzers. CheckScoreUsageTest passes all 20 tests. The full FE build passes with Checkstyle reporting zero violations.

  • Behavior changed:

    • No.
    • Yes. score() plans for non-scoring SNII indexes now fail during FE analysis; index writes and the storage format are unchanged.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

### What problem does this PR solve?

Issue Number: None

Related PR: apache#66052

Problem Summary: FE admitted score() plans for every phrase-enabled SNII inverted index, but built-in analyzers and ordinary custom analyzers write only the docs-and-positions tier. They do not persist norms or semantic scoring metadata, so the BE later failed while opening scoring statistics. Reject those plans at the selected-index admission gate and admit SNII scoring only for indexes created with a CommonGrams analyzer, which writes the scoring tier. Existing V3 behavior is unchanged.

### Release note

Reject score() queries on SNII indexes that do not persist scoring data instead of failing later in the BE.

### Check List (For Author)

- Test: Unit Test
    - Added end-to-end score pushdown cases for built-in, plain custom, and CommonGrams SNII analyzers
    - Ran CheckScoreUsageTest: 20 tests passed
    - Ran the full FE build with 192-way parallelism
- Behavior changed: Yes, score() plans for non-scoring SNII indexes now fail during FE analysis; index writes and the storage format are unchanged
- Does this need documentation: No
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@airborne12

Copy link
Copy Markdown
Member Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 17020 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 4598fc28fd965a7dc0834855664f3db41ec04d73, data reload: false

------ Round 1 ----------------------------------
orders	Doris	NULL	NULL	0	0	0	NULL	0	NULL	NULL	2023-12-26 18:27:23	2023-12-26 18:42:55	NULL	utf-8	NULL	NULL	
============================================
q1	17599	3086	3076	3076
q2	q3	10891	833	512	512
q4	4682	257	209	209
q5	7793	570	395	395
q6	137	114	93	93
q7	537	511	399	399
q8	9261	898	992	898
q9	3571	2390	2351	2351
q10	6537	856	714	714
q11	769	274	248	248
q12	668	394	352	352
q13	17951	1548	1162	1162
q14	160	149	142	142
q15	q16	480	391	364	364
q17	816	778	753	753
q18	3193	2235	2266	2235
q19	1296	925	717	717
q20	728	531	475	475
q21	5560	1698	1896	1698
q22	330	267	227	227
Total cold run time: 92959 ms
Total hot run time: 17020 ms

----- Round 2, with runtime_filter_mode=off -----
orders	Doris	NULL	NULL	150000000	42	6422171781	NULL	22778155	NULL	NULL	2023-12-26 18:27:23	2023-12-26 18:42:55	NULL	utf-8	NULL	NULL	
============================================
q1	3514	3370	3368	3368
q2	q3	2212	2330	2161	2161
q4	1186	1171	893	893
q5	2175	2118	2111	2111
q6	169	114	89	89
q7	1048	956	897	897
q8	1608	1413	1431	1413
q9	3136	3073	3090	3073
q10	1826	1764	1599	1599
q11	360	277	271	271
q12	453	421	342	342
q13	1491	1537	1136	1136
q14	175	177	159	159
q15	q16	398	398	386	386
q17	1051	1041	1032	1032
q18	4888	4365	4727	4365
q19	849	812	807	807
q20	973	924	830	830
q21	3754	3074	3203	3074
q22	394	345	332	332
Total cold run time: 31660 ms
Total hot run time: 28338 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 81500 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 4598fc28fd965a7dc0834855664f3db41ec04d73, data reload: false

query5	4255	428	355	355
query6	536	159	160	159
query7	4834	466	266	266
query8	376	131	120	120
query9	8785	2898	2884	2884
query10	510	265	228	228
query11	5450	1049	927	927
query12	136	81	72	72
query13	1227	460	325	325
query14	6139	2215	2077	2077
query14_1	2016	1988	1977	1977
query15	182	118	113	113
query16	3197	413	398	398
query17	916	457	376	376
query18	2176	337	260	260
query19	179	150	116	116
query20	92	68	70	68
query21	826	119	104	104
query22	5570	5433	5333	5333
query23	6616	6099	6249	6099
query23_1	5994	6160	6366	6160
query24	7368	1115	755	755
query24_1	780	764	743	743
query25	528	280	232	232
query26	1220	266	162	162
query27	2733	459	274	274
query28	4602	1505	1502	1502
query29	1021	430	337	337
query30	434	177	154	154
query31	888	440	360	360
query32	135	48	47	47
query33	517	216	186	186
query34	1166	839	500	500
query35	503	409	346	346
query36	566	556	517	517
query37	131	83	69	69
query38	1037	867	1027	867
query39	540	517	485	485
query39_1	502	501	512	501
query40	276	118	112	112
query41	53	53	58	53
query42	82	107	81	81
query43	251	249	218	218
query44	
query45	111	110	96	96
query46	825	862	547	547
query47	774	787	729	729
query48	304	307	229	229
query49	578	233	220	220
query50	825	327	282	282
query51	7919	7920	8054	7920
query52	74	72	67	67
query53	231	215	163	163
query54	241	195	170	170
query55	101	57	54	54
query56	268	233	225	225
query57	807	680	615	615
query58	237	200	182	182
query59	1242	1271	1097	1097
query60	301	205	203	203
query61	117	127	109	109
query62	493	211	182	182
query63	186	157	152	152
query64	2313	707	593	593
query65	
query66	1734	311	267	267
query67	10353	10131	10082	10082
query68	
query69	684	227	195	195
query70	627	631	619	619
query71	317	245	242	242
query72	3018	1887	1758	1758
query73	732	613	352	352
query74	1632	1212	1128	1128
query75	1220	1124	1021	1021
query76	1887	721	539	539
query77	260	255	201	201
query78	3937	3776	3380	3380
query79	1594	774	601	601
query80	1232	394	340	340
query81	514	200	175	175
query82	1177	147	105	105
query83	331	251	234	234
query84	
query85	1177	505	477	477
query86	675	171	168	168
query87	1073	983	903	903
query88	2927	2152	2187	2152
query89	347	233	206	206
query90	2082	153	147	147
query91	186	138	127	127
query92	49	45	43	43
query93	1447	1166	794	794
query94	1008	259	218	218
query95	677	354	354	354
query96	848	581	282	282
query97	1038	1099	1028	1028
query98	151	133	132	132
query99	580	355	320	320
Total cold run time: 180839 ms
Total hot run time: 81500 ms

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 1.45% (2/138) 🎉
Increment coverage report
Complete coverage report

@airborne12

Copy link
Copy Markdown
Member Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: the new SNII score admission has three P1 correctness gaps. It uses current analyzer policy as a proxy for persisted segment capability, applies capability checks to predicates that do not contribute BM25, and validates a generic SEARCH index that can differ from the runtime index for EXACT.

Critical checkpoint conclusions:

  • Goal and proof: The narrow mocked built-in/plain cases are covered, but the claimed end-to-end rejection is not established; CommonGrams data can still fail later, while valid mixed filters can now be rejected.
  • Scope and clarity: The diff is small, but the helper conflates policy identity, score-contributor classification, physical-index routing, and persisted capability.
  • Concurrency: No new threads or mutable shared state are introduced. Existing IndexPolicyMgr read locking is sound; no race, lock-order, or deadlock issue was found.
  • Lifecycle: Policy drop/replay is protected, but legacy, build-disabled, mixed, rolling-upgrade, and replica-divergent segments outlive the current analyzer classification (inline issue).
  • Configuration: No configuration is added. Existing dynamic enable_common_grams_index_build is snapshotted per writer and invalidates analyzer-only admission.
  • Compatibility: No protocol or storage-format change is added and V3 behavior is structurally unchanged, but old/pre-metadata SNII segments are not conservatively handled.
  • Parallel and conditional paths: MATCH and SEARCH were both traced. Non-scoring SEARCH/direct-MATCH leaves and EXACT multi-index routing are the two path-specific inline issues; other supported score-bearing routes align.
  • Tests and results: The new JUnit cases are deterministic but mock policy results and miss persisted missing/mixed segments, contributor mixtures, and EXACT multi-index selection. CI FE UT, CheckStyle, compile, and regression checks observed during review were green; no local build or tests were run because this review environment forbids them.
  • Observability and errors: Error conversion/context is adequate; no additional metric or logging issue was found.
  • Transactions, persistence, writes, and FE/BE variables: No transaction, edit-log, data-write, or new protocol-variable path is modified.
  • Performance and resource safety: Planning-time lookups are bounded; no material CPU, memory, nullability, or resource issue was found.
  • Other/final sweep: All three review rounds converged with no unresolved candidate beyond the three inline findings.

User focus: no additional focus was provided.

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 0.17% (2/1204) 🎉
Increment coverage report
Complete coverage report

@airborne12

airborne12 commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

(Edited: the original wording of this note was wrong about the outcome. See the follow-up comment for the decision and the reasoning behind it.)

#67134 decouples SNII BM25 scoring from CommonGrams, which invalidates the premise this PR's new rejections rest on. CommonGrams is a phrase-query performance optimization; scoring was coupled to it only because the semantic collection statistics BM25 needs were introduced inside the CommonGrams segment metadata. V1/V2/V3 rank an ordinary PROPERTIES("parser"="english","support_phrase"="true") index (test_bm25_score.groovy), so SNII was the outlier. After #67134 any analyzed SNII index with positions persists norms and is rankable — including ARRAY columns, which CommonGrams rejects outright and which therefore could never be ranked on SNII at all.

So the two rejections this PR adds would refuse queries that now work, and need to be narrowed to what remains true: an SNII index that is not analyzed at all (no parser, no analyzer) still has no scoring data, exactly as on V1/V2/V3.

Status of the three review findings:

  • [P1] Check only predicates that contribute to score — confirmed. SearchPredicateCollector::is_score_query_type admits only TERM/EXACT/PHRASE/MATCH/ANY/ALL, and IndexReaderHelper::is_need_similarity_score only MATCH_ANY/ALL/PHRASE/PHRASE_PREFIX, so a PREFIX leaf or a MATCH_REGEXP predicate contributes nothing and is silently skipped by the BE. I reproduced the over-rejection at FE unit level for both the SEARCH and the direct-MATCH shape. Whatever admission check survives here must mirror that classification instead of rejecting every SNII leaf in the filter.

  • [P1] Gate on persisted scoring capability, not analyzer type — correct, and the reason this rule sat at the wrong layer. test_common_grams_snii.groovy already constructs a CommonGrams-analyzer table whose segments were written with enable_common_grams_index_build=false and asserts the BE-side failure, so the analyzer graph never was proof that scoring data exists. [fix](be) Decouple SNII BM25 scoring from CommonGrams #67134 makes the persisted capability (scoring tier + positions + norms) the single predicate, in the BE.

  • [P1] Resolve SEARCH admission with the runtime query type — this one is independent of scoring and survives. On a column carrying both an untokenized index and an analyzed one, OlapTable.getInvertedIndex(column, subPath, null) returns the first index for which isAnalyzedInvertedIndex() is true, regardless of declaration order, while the BE routes an EXACT clause to InvertedIndexReaderType::STRING_TYPE (inverted_index_selector.cpp, is_equal_query branch). FE and BE can therefore select different physical indexes for the same leaf. test_search_exact_multi_index.groovy shows the multi-index-per-column shape is supported and tested. Worth its own issue.

Thanks for the review — right on all three counts.

@airborne12 airborne12 closed this Aug 25, 2026
@airborne12 airborne12 reopened this Aug 25, 2026
@airborne12

Copy link
Copy Markdown
Member Author

Closing. The decision comes down to a behaviour comparison with V1/V2/V3, which I should have run before opening this.

What V1/V2/V3 do when score() meets an index that cannot be ranked: nothing. There is no rejection anywhere in that path.

  • FE: the only index-related rejection in CheckScoreUsage is usesCommonGrams && !isSnii. A keyword index, or an analyzed index built without support_phrase, is admitted without comment.
  • BE, keyword index: it is served by StringTypeInvertedIndexReader, which has no similarity path at all — the field simply contributes no score.
  • BE, analyzed index without positions: IndexReaderHelper::is_need_similarity_score returns false, query_info.is_similarity_score is never set, and again nothing is scored.

An unscoreable index is a field that contributes no relevance, not an error. That matches every mainstream engine — Elasticsearch's filter/must_not clauses return score 0 and never invalidate the query, Lucene's multi-term queries fall back to a constant score, Solr's fq restricts "without influencing score" — and it is the contract the rest of Doris already implements.

With #67134 in, SNII lines up with that contract almost exactly:

shape V1/V2/V3 SNII after #67134
analyzed + positions ranks ranks
analyzed, support_phrase=false silently no score silently no score (same is_need_similarity_score)
keyword (no parser, no analyzer) silently no score errors

Adding an FE rejection for SNII would make the first two rows diverge, not converge: the same SQL would return rows on a V3 table and fail at analysis time on an SNII one. That is the opposite of what this PR set out to achieve.

The one genuine divergence left is the third row, and it is a BE issue, not an FE one: is_need_similarity_score keys off support_phrase alone and ignores should_analyzer, so a keyword index declared with support_phrase=true raises actual_similarity, SNII then opens its stats provider and fails, while V2/V3 was never on that code path because a keyword index uses a different reader class. Fixing that means teaching SNII to skip silently, in the BE — I will carry it as follow-up to #67134.

Also carrying forward, independent of scoring: the FE/BE index-resolution divergence from the third review thread. OlapTable.getInvertedIndex(column, subPath, null) returns the first index for which isAnalyzedInvertedIndex() is true regardless of declaration order, while the BE routes an EXACT clause to InvertedIndexReaderType::STRING_TYPE, so the two can select different physical indexes for the same leaf. test_search_exact_multi_index.groovy shows that multi-index-per-column shape is supported and tested. That one deserves its own issue.

Thanks for the review — all three findings were correct, and two of them are what showed this rule belonged one layer down.

@airborne12 airborne12 closed this Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants