tantivy icon indicating copy to clipboard operation
tantivy copied to clipboard

fix: boolean query incorrectly dropping documents when AllScorer is present

Open mdashti opened this issue 4 months ago • 1 comments

What

Fixed a bug where Boolean queries would incorrectly return 0 results (or fewer than expected) when one of the clauses produced an AllScorer—typically from range queries matching all documents.

Why

When BooleanWeight processes scorers, it removes AllScorer instances as an optimization (since they match everything). However, the code wasn't properly accounting for these removed scorers in certain MUST/SHOULD combinations, causing:

  1. SHOULD + SHOULD with AllScorer: The "match all" semantics were lost, leaving only the other SHOULD clause
  2. MUST (AllScorer) + SHOULD: Empty must_scorers vector → intersect_scorers([]) returned EmptyScorer
  3. Range queries in Boolean clauses: When all docs match a range (e.g., age > 50 when everyone is over 50), the query would silently fail

This commonly surfaced in queries like:

(age > 50 OR name = 'Alice') AND status = 'active'

when all documents happened to satisfy the range condition.

How

Introduced two helper functions to centralize the AllScorer handling logic:

effective_must_scorer() — Returns the effective MUST scorer accounting for removed AllScorers:

  • Empty list + had AllScorers → returns AllScorer (all docs match)
  • Empty list + no AllScorers → returns None (no MUST constraint)
  • Non-empty list → returns intersection of scorers

effective_should_scorer_for_union() — Restores "match all" semantics for SHOULD:

  • If AllScorers were removed, unions the remaining scorer with AllScorer

The complex_scorer() match arms now use these helpers consistently:

  • Ignored: Use effective MUST scorer or empty
  • Optional: No MUST → promote SHOULD (with union fix); Has MUST → SHOULD only affects scoring
  • Required: No MUST → SHOULD alone; Has MUST → intersect MUST with SHOULD

Tests

Added 4 regression tests.

mdashti avatar Dec 04 '25 06:12 mdashti

interestingly, it seems it's not possible to trigger this bug with an AllQuery (only 1 of the added tests fails on previous commits, the range one) because it actually generates a BoostScorer<AllScorer>, not an AllScorer. It's possible to trigger it with a Range over fastfield, an exist query, and even a TermQuery.

This means we may be missing some optimisations in some case, without knowing it.

I also notice there seems to be places where scoring might be wrong due to AllScorer handling, notably FastFieldRangeWeight doesn't always honor boost (unbounded range on both sides), and BooleanWeight remove most AllScorers without compensating scores. That last one can cause result reordering on some score combiner inside a single segment, and on any combiner when considering multiple segments, where only some segment sees AllScorers, and other see a Range or Exist scorer

trinity-1686a avatar Dec 04 '25 23:12 trinity-1686a

@fulmicoton @PSeitz any more comments?

mdashti avatar Dec 10 '25 18:12 mdashti

@stuhood Can you rebase/fix the conflict?

fulmicoton avatar Dec 14 '25 09:12 fulmicoton

@stuhood Can you rebase/fix the conflict?

Done. Thanks!

stuhood avatar Dec 15 '25 18:12 stuhood

Thank you!

fulmicoton avatar Dec 16 '25 21:12 fulmicoton