Skip to content

SOLR-18625: eDisMax: don't emit empty pf or bq clauses - #5303

Open
ispringer wants to merge 4 commits into
apache:mainfrom
ispringer:SOLR-18625
Open

ispringer wants to merge 4 commits into
apache:mainfrom
ispringer:SOLR-18625

Conversation

@ispringer

@ispringer ispringer commented Nov 5, 2026 •

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/SOLR-18625

Description

When pf is set and no phrase qualifies for the boost (single-term queries, q=*:*), eDisMax's pf sub-parser discards every phrase via minClauseSize and returns an empty BooleanQuery rather than null. That passed the phrase != null guard and was added to the main query as an empty SHOULD clause (visible as () in the parsed query, e.g. +(subject:wireless) ()).

It did not affect scoring, but it defeated the single-clause MatchAllDocsQuery unwrapping in ExtendedDismaxQParser.parse(), so q=*:* with pf set scored a BooleanQuery over every doc instead of reaching Solr's MatchAllDocsQuery fast paths.

The same happens with bq: a boost query consisting only of stopwords (e.g. bq=text:a) parses to an empty BooleanQuery and was added unchecked, with the same effect on q=*:*.

Split out from #4652 (closed), where the pf part was agreed to be safe on its own; the bq part is the same fix applied to the other path that can add an empty clause.

Solution

In addShingledPhraseQueries:

  • Skip adding the phrase clause when the parsed result is empty, not only when it is null.
  • Return early when there are no clauses (e.g. fielded-only queries like q=subject:foo), avoiding creating a sub-parser and analyzing an empty phrase per pf slop group.

In getBoostQueries, skip a boost query that parses to an empty query.

No ranking change.

Co-authored with Claude Code (Opus 5.5).

Out of scope

DisMaxQParser.addBoostQuery has the same gap for bq (it already uses ExtendedDismaxQParser.isEmpty for pf but not bq). Left out to keep this PR focused on eDisMax; it would be a trivial follow-up if wanted.

Tests

TestExtendedDismaxParser (new, each run with sow=true and sow=false):

  • testNoEmptyPhraseBoostClause: q=wireless → no pf boost and no empty () clause (fails without the fix).
  • testNoEmptyPhraseBoostClauseForFieldedQuery: q=subject:wireless → same, exercising the no-clauses early return.
  • testAnalyzedSingleTermKeepsPhraseBoost: q=wi-fi (WordDelimiter splits into two tokens) → phrase boost retained.
  • testSingleTermSynonymKeepsPhraseBoost: q=grackle with qf=id → multi-term synonym phrase boost retained.

TestExtendedDismaxParser.testMatchAllDocs (extended): now sets pf, pf2, pf3 and a stopword-only bq, across q=*:*/q=* and both sow values, and still expects MatchAllDocsQuery (fails without the bq fix).

Ran TestExtendedDismaxParser, QueryEqualityTest, DisMaxRequestHandlerTest (all green), plus ./gradlew tidy and ./gradlew check -x test.

Checklist

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide
  • I have added a changelog entry for my change

🤖 Generated with Claude Code

When no phrase qualifies for the pf boost (single-term queries, q=*:*),
the pf sub-parser returns an empty BooleanQuery that was added to the
main query as a SHOULD clause, defeating the single-clause
MatchAllDocsQuery optimization. Omit it, and return early when there
are no clauses to build a phrase from. Likewise skip a bq that parses
to an empty query, e.g. one consisting only of stopwords.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
@ispringer ispringer changed the title SOLR-18625: eDisMax: don't emit an empty pf clause SOLR-18625: eDisMax: don't emit empty pf or bq clauses Nov 5, 2026
@nick-boss-tech

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖 (reviewed by Nick Shanin)

@ispringer I prepared an optional test-only follow-up for your current version. It adds plain-token and fielded-only checks in both sow modes, plus independent wi-fi and configured grackle synonym-preservation tests. It retains your expanded pf/pf2/pf3 and stopword-only bq MatchAll coverage. Only TestExtendedDismaxParser.java changes.

Selected suites passed on your head fd106e0 (157 tests) and the follow-up 07328d6 (160 tests), with no failures, errors or skips, seed 5303C0DE, Linux/JDK 21. Root tidy, Error Prone compilation and root check -x test passed at the follow-up head with NODE_ENV unset. Five compiling mutation controls failed at the intended assertions, including false-mode-only regressions. These are selected-suite results, not a full-suite or performance claim. Verification details.

If useful, the test-only diff is one commit directly above your current head and can be taken without rewriting history:

git switch SOLR-18625
git fetch https://github.com/nick-boss-tech/solr.git pr-5303-phrase-coverage-round-2
git merge --ff-only 07328d6973c4bb3385ca6d50bfe871f8808c1550

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@ispringer

Copy link
Copy Markdown
Author

Thanks @nick-boss-tech, took it. Fast-forwarded 07328d6 as-is, then added 901cc74 on top which collapses the four sow loops into a shared assertPhraseBoostParsedQuery helper and tags each assertion with the sow mode. TestExtendedDismaxParser passes locally with the changes.


Written with Claude Code (Fable 5.1)

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants