Repository navigation
Conversation
…lysis entirely for single-term queries, since a one-word phrase is semantically meaningless
…the removal of the single-term phrase boost
dsmiley
left a comment
There was a problem hiding this comment.
I'm rather surprised to see this had no impact on the tests. There should definitely be a test.
I suppose some people will notice a scoring impact... but this seems like the correct behavior here -- so be it. The parser shouldn't be frozen.
…g early due to single term; add brief comments explaining reason for early returns
|
I had some doubts so I shared them with Claude Opus for analysis. Here is the report. I checked two things here: (a) whether pf2/pf3 already short-circuit so that this is merely the same rule extended to pf, and (b) whether the changelog's relevancy caveat is accurate. (a) holds; (b) does not, and the counterexample also invalidates the premise of the change itself. The pf2/pf3 short-circuit does already exist
if (null == fields || fields.isEmpty() || null == clauses || clauses.size() < shingleSize) return;
So the shape of the change is consistent. It additionally covers But "single term" is being detected as "no whitespace", which is not the same thingParsed queries on current
The " Analyzer chains where a whitespace-single-token query still yields a meaningful phrase boost:
The changelog caveat is too narrow
The The caveat is correct in the narrow case it was presumably written for — identical fields, identical boosts, and one-token-in/one-token-out analysis. There the boost clause is identical to the qf clause, the score doubles uniformly, and rank order is preserved. The DisMax and eDisMax halves are not the same riskThese are described together, but DisMax has no SuggestionIf the goal is the optimization, gate on the analyzed token count rather than on whitespace. For eDisMax specifically, |
…hrase MatchAllDocs defeat Detect single-term phrase (pf) boosts from the analyzed query rather than raw-string whitespace, so DisMax no longer drops meaningful phrase boosts for whitespace-free queries that analyze into multiple tokens (CJK, ngram, WordDelimiterGraph, multi-word synonyms). eDisMax already skipped single-term boosts via minClauseSize; remove its redundant/flawed shingleSize guard. Also fix eDisMax adding an empty pf SHOULD clause for clause-less queries such as q=*:*, which defeated the single-clause MatchAllDocsQuery optimization. Co-Authored-By: Claude <[email protected]>
Make DisMaxQParser.isEffectivelySingleTerm package-private (@VisibleForTesting) and add TestDisMaxQParser covering the single-term, multi-term, mixed-DisMax, unrecognized-shape, and null cases that are awkward to reach end-to-end. Co-Authored-By: Claude <[email protected]>
…Test Add a "usa" => "united states of america" case exercising the DisMax parser's MultiPhraseQuery branch end-to-end: a single input term expanded by a multi-word synonym analyzes into multiple tokens and must retain its pf phrase boost. Adds a text_syn field (existing synonym-enabled lowerpunctfilt type) to the shared collection1 schema. Co-Authored-By: Claude <[email protected]>
| } | ||
| } | ||
|
|
||
| protected Query getPhraseQuery(String userQuery, SolrPluginUtils.DisjunctionMaxQueryParser pp) |
There was a problem hiding this comment.
Idea: I'm thinking that we want to know how many clauses are are in the BooleanQuery of the parsed userQuery. If it's one (or no BQ) then there were no whitespace separated components, and no need for phrase boost.
At this stage right here, we only have the String version of userQuery, so we don't know. Rather than do parser-like things here by inspecting the string, we can probably avoid this as redundant/hacky given we're going to already parse teh userQuery anyway. We can inspect a parsedQuery. Inspecting a String is not idea.
So where I'm going with this is, update the caller (who has the parsedUserQuery) to inspect it and conditionally call getPhraseQuery. Then I think you won't even be modifying getPhraseQuery.
There was a problem hiding this comment.
Opus advises against reusing the parsed q / counting BQ clauses for two reasons:
- Clause count ≈ whitespace count. The qf parser has setSplitOnWhitespace(true), so BQ clause count is really the number of whitespace-separated words, not analyzed terms. That skips the boost for wi-fi, CJK, WordDelimiterGraph, multi-word synonyms, etc. — all "one word" but multiple analyzed terms that genuinely want the phrase boost.
- q is the qf parse, the boost is the pf parse. parsedUserQuery is analyzed with the qf fields; the phrase boost is analyzed with the pf fields, force-quoted. When pf ≠ qf they can differ in fields/analyzers (and shape, since one's free-form and one's a forced phrase), so the qf query's term count isn't a reliable proxy.
From me: It seems better to favor correctness for all the corner cases over the perf gain from skipping the pp parse. Do you agree?
There was a problem hiding this comment.
- I don't get the problem. "so BQ clause count is really the number of whitespace-separated words, not analyzed terms. " -- so what? I think we indeed want to know the clause count of 'q' (i.e. BQ clause count) and don't care what the analyzed term shape looks like. If the query is
wi-fithen this is one clause and will end up producing a PhraseQuery of "wi fi" because the non-whitespace span of characters implies positional adjacency to analyzed component terms. - "so the qf query's term count isn't a reliable proxy." I agree and didn't mean to imply using that. I only suggest examining the clause count of the parsedUserQuery, which is loosely the number of whitespace separated parts. Technically, maybe not exactly this, since the query string may have syntax like quotes, thus the whitespace inside quoted parts don't count. But the parser accounts for that, emitting a BooleanQuery clause for each component, thus it'd just be a PhraseQuery if a user quoted their query already.
There was a problem hiding this comment.
You're right on (2), I misread your suggestion, sorry about that.
On (1), the piece I failed to spell out is autoGeneratePhraseQueries. The "wi-fi already becomes "wi fi"" behavior only happens when the qf field type has it set to true. It has defaulted to false since schema 1.4 (TextField), and in the _default configset only text_en_splitting* enable it; text_general and text_cjk don't. With it false, the qf clause for wi-fi is (wi fi), a plain disjunction, so the pf boost is the only adjacency signal, and gating on clause count would drop it. CJK with CJKBigram is the worst case: every query is a single clause, so clause-count gating would disable pf for essentially all CJK queries.
Checking the analyzed pf phrase also keeps DisMax consistent with eDisMax, whose minClauseSize gate counts analyzed terms in the pf phrase rather than clauses, so eDisMax keeps pf for wi-fi too.
I did take your structural suggestion: the check now lives in addMainQuery, and getPhraseQuery is unchanged from main (6590729). I also added a DisMax test case on a field with autoGeneratePhraseQueries="false" showing wi-fi keeps its pf boost, and confirmed it fails if the gate is switched to clause count. The existing wi-fi/usa assertions were also matching the qf clause (the test schema is version 1.0, so the flag defaults to true there), so I tightened them to match the pf clause specifically.
(Analysis and changes co-authored with Claude Code, Opus 5.5.)
|
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution! |
…raseQueries=false Move the single-term pf check from getPhraseQuery into addMainQuery, per review, so getPhraseQuery is unchanged from main and subclass overrides still get the gate. Add a subword_noautophrase field type (autoGeneratePhraseQueries="false", the default since schema 1.4) and a DisMax case showing q=wi-fi parses its qf clause as a plain disjunction, so the pf phrase boost is the only adjacency signal and must be kept. Tighten the existing wi-fi and synonym assertions to match the pf clause specifically (its ps slop and boost); they previously also matched the qf clause. Co-authored with Claude Code (Claude Opus 5.5). Co-Authored-By: Claude <[email protected]>
…m shapes; tighten tests Address code review of the DisMax single-term pf gate: - Only skip the pf boost when the user query is a single whitespace-free term AND it analyzes to a single position. A multi-word query on a keyword or string pf field analyzes to one TermQuery but is an exact-match boost that must be kept. - Treat single-position synonyms as single-term regardless of synonymQueryStyle (SynonymQuery, pick_best DisjunctionMaxQuery, or as_distinct_terms all-SHOULD BooleanQuery), matching eDisMax's minClauseSize behaviour. - Drop the "redundant term boost" rationale from comments and changelog: pf fields not in qf lose their only boost, so say so. eDisMax: return early from addShingledPhraseQueries when there are no clauses instead of building and parsing an empty phrase, and reuse the existing isEmpty(Query) helper. Note the empty-clause fix also applies to single-term queries in the changelog. Tests: make the DisMax single-term assertion check for the pf clause itself (it previously could not fail); add a string-field exact-match case; cover the synonym shapes in TestDisMaxQParser and move it to SolrTestCase; close the inline request in TestExtendedDismaxParser. Ref guide: note that the DisMax pf boost is not applied to single-term queries. Co-authored with Claude Code (Fable 5.1). Co-Authored-By: Claude <[email protected]> Co-Authored-By: Claude Fable 5.1 <[email protected]>
…y; document ranking impact Drop the BooleanQuery branch from DisMaxQParser.isEffectivelySingleTerm: the all-SHOULD BooleanQuery that synonymQueryStyle=as_distinct_terms builds for single-position synonyms is not subject to eDisMax's minClauseSize gate, so treating it as single-term made DisMax drop a boost eDisMax keeps. Also guard against an empty DisjunctionMaxQuery being vacuously single-term. Ref guide: note that a pf-only field no longer affects single-term DisMax scores and point to qf/bq; add the change to major-changes-in-solr-11. Co-authored with Claude Code (Fable 5.1). Co-Authored-By: Claude <[email protected]> Co-Authored-By: Claude Fable 5.1 <[email protected]>
|
@dsmiley - can you take another look when you have a chance? thanks! |
This PR makes two related but independent changes to phrase field (
pf) boosting in the DisMax and eDisMax query parsers.(co-authored with Claude Code: Opus 4.8, Opus 5.5 and Fable 5.1)
1. DisMax: skip the pf boost for single-term queries (
changed)The DisMax parser no longer applies the
pfphrase boost to single-term queries, matching eDisMax (which already skips them via itsminClauseSizegate). A one-position phrase imposes no adjacency constraint, so it is just a term boost.A query counts as single-term only when both:
TermQuery, or single-position synonyms as built bysynonymQueryStyle=as_same_term(SynonymQuery) orpick_best(DisjunctionMaxQuery). TheBooleanQuerythatas_distinct_termsbuilds is kept, because eDisMax'sminClauseSizegate also keeps it — the two parsers agree for every style.So these keep their boost:
This may change relevancy scores for single-term DisMax queries, including those whose pf fields are not also in qf (such fields previously contributed a term boost via pf alone).
2. eDisMax: don't emit an empty pf clause (
fixed)When no phrase qualifies for the boost (single-term queries,
q=*:*),minClauseSizediscards every phrase and parsing returns an emptyBooleanQuery, which was added to the main query as an emptySHOULDclause. It did not affect scoring but defeated the single-clauseMatchAllDocsQueryoptimization; it is now omitted, soq=*:*reduces to aMatchAllDocsQueryeven whenpfis set. Fielded-only queries (e.g.q=subject:foo) also now return early instead of building and parsing an empty phrase.Notes
DisjunctionMaxQueryclause, avoiding a second postings read and scoring pass per pf field for every matching doc.q=*:*withpfnow reaches Solr'sMatchAllDocsQueryfast paths (answered from the live-docs set, bypassing the filterCache) instead of scoring aBooleanQueryover every doc.pfsection now notes that single-term queries get nopfboost and that apf-only field therefore contributes nothing to a single-term query's score (useqforbqinstead); the same note is inmajor-changes-in-solr-11.adocsince this is a relevancy change with no opt-out.minClauseSize. Converging the two parsers' multi-word behavior is out of scope.Tests
Tests added
TestDisMaxQParser(new,SolrTestCase) — unit tests forDisMaxQParser.isEffectivelySingleTerm(package-private via@VisibleForTesting), exercising the query-shape logic directly without a running core:BoostQuery-wrappedTermQuery; one-position synonyms as built byas_same_term(SynonymQuery) andpick_best(DisjunctionMaxQuery); degenerate one-termPhraseQuery/MultiPhraseQuery; an all-single-termDisjunctionMaxQueryacross fields.PhraseQuery, two-positionMultiPhraseQuery, a sloppy graph phrase (BooleanQueryof per-path phrases), and a mixedDisjunctionMaxQuerywhere one field is multi-term.MatchAllDocsQuery, theas_distinct_termsall-SHOULDBooleanQuery(eDisMax keeps it too), aBooleanQuerywith a required clause, an emptyDisjunctionMaxQuery, andnull(must not throw).DisMaxRequestHandlerTest.testSingleTermPhraseBoost(new) — end-to-end through the classic DisMax parser:q=wireless(single analyzed token) → the qf clause is present and the pf clause(subword:wireless)^10.0is not.q=wi-fi(WordDelimiter splits intowi fi) → pf phrase boost retained.q=wi-fion a field withautoGeneratePhraseQueries="false"(the default since schema 1.4) → the qf clause is a plain disjunction, so the pf phrase boost is the only adjacency signal and is retained. Added asubword_noautophrasefield to the sharedcollection1schema to drive this.q=usa(multi-word synonym →united states of america) → pf phrase boost retained. Added atext_synfield (existing synonym-enabled type) to the sharedcollection1schema.q=hello worldwithpf=foo_s^10(string field) → the exact-match boost(foo_s:hello world)^10.0is retained even though it analyzes to a single term.psslop andpfboost), since withautoGeneratePhraseQueries="true"the qf clause is the same phrase.TestExtendedDismaxParser.testSingleTermPhraseBoost(new) — end-to-end through eDisMax:q=wireless→ no pf boost and no empty()clause;q=wi-fiandq=usa→ boost retained (viaminClauseSize).q=*:*→ reduces toMatchAllDocsQuery(regression guard for the empty-clause fix).(CJK was intentionally not given a dedicated test — a single CJK input term yields the same multi-term
PhraseQuerythewi-fi/usacases already exercise, so it adds no code-path coverage while requiring a CJK analyzer in the test schema.)Regression checks run
All green (319 tests across 18 suites):
TestDisMaxQParser,DisMaxRequestHandlerTest,TestExtendedDismaxParser,TestMaxScoreQueryParser,QueryEqualityTest— the parser suites directly touched.solr/coresuite that exercises the DisMax parser:QueryElevationComponentTest,HighlighterTest,RankFieldTest,DistributedQParserTest,TestFunctionQuery,TestAuxIndexJoinQParserPlugin,TestScoreJoinQPNoScore,TestQueryTypes,TestSearchPerf,SpellCheckCollatorTest,TestJoin,SolrPluginUtilsTest, plusBasicFunctionalityTestfor the additivetext_syn/subword_noautophrasefields in the sharedcollection1schema../gradlew tidyand./gradlew check -x test— clean.Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.🤖 Generated with Claude Code