Repository navigation
Conversation
Add scored SimpleUnion support using the existing ScoreCombiner interface. Select it for unions with at most 128 estimated hits per 4096 documents, and retain buffered traversal for denser unions. Apply the selection to scalar, unscored, and non-Block-WAND pruning paths. Preserve aligned scorer construction, frequency requirements, single-term shortcuts, and eligible Block-WAND pruning. Keep scored batches, parent selectivity plumbing, and the separate no-score combiner fix outside this change. Cover sum/dismax score association, repeated scoring and seeks, threshold boundaries, sparse flat/nested real-index queries, and single-term frequency specialization. The density threshold remains unbenchmarked.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47522b46a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let estimated_hits = estimate_union(scorers.iter().map(DocSet::size_hint), num_docs); | ||
| let strategy = union_strategy_for_estimated_hits(estimated_hits, num_docs); |
There was a problem hiding this comment.
On segments with deletions or a custom alive bitset, num_docs is only the live count, while TermScorer::size_hint() is the full postings length and the union still traverses deleted document IDs. A posting size can therefore exceed the denominator used by estimate_union, producing probabilities above 1; with multiple clauses the estimate can even saturate to 0 and select SimpleUnion, whose advance scans every child for every hit, causing a severe regression for wide unions. Base both the estimate and threshold comparison on reader.max_doc() (or otherwise normalize sizes and the denominator to the same population).
Useful? React with 👍 / 👎.
Remove all five tests previously added by this PR and preserve upstream tests. Generate sparse and dense postings to check actual strategy dispatch, exact sum/dismax score parity against buffered union, and seek/advance cursor behavior across buffer windows. Keep production code unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: feff4d37d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let mut union_scorer = | ||
| BufferedUnionScorer::build(term_scorers, &self.score_combiner_fn, num_docs); | ||
| for_each_pruning_scorer(&mut union_scorer, threshold, callback); | ||
| build_union(term_scorers, strategy, &self.score_combiner_fn, num_docs); | ||
| for_each_pruning_scorer(union_scorer.as_mut(), threshold, callback); |
There was a problem hiding this comment.
For dense all-term queries that fall back to ordinary pruning (notably DisjunctionMaxQuery, whose combiner has SUPPORTS_BLOCK_WAND = false), build_union returns Box<dyn Scorer> and this loop now invokes doc/score/advance through a vtable for every matching document. The previous TermUnion arm kept BufferedUnionScorer concrete when calling the generic traversal helper, so high-hit disjunctions lose that hot-loop specialization; branch on strategy before entering the loop (and do the same in for_each) to retain static dispatch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 990955a63f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Child scorers retain density selection for nested queries that use seeking. | ||
| let scorers = should_weights | ||
| .iter() | ||
| .map(|weight| weight.scorer(reader, 1.0)) |
There was a problem hiding this comment.
When Count runs on a pure OR containing an explicitly nested Boolean OR, this call builds each child through its normal scorer path, which uses the 128/4096 traversal cutoff rather than COUNT_UNION_MAX_EXPECTED_HITS_PER_HORIZON. The parent then calls count_including_deleted, which exhausts child unions with advance/refill and does not seek, so a 1–3%-dense nested union remains a SimpleUnion and pays the per-hit scan that the new count cutoff was intended to avoid. Propagate the count strategy into nested scorer construction or flatten eligible nested ORs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfebb65529
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if scorers.len() == 1 && !scorers[0].is::<TermScorer>() { | ||
| return SpecializedScorer::Other(scorers.into_iter().next().unwrap()); //< we checked the size beforehand | ||
| } | ||
| let estimated_hits = estimate_union(scorers.iter().map(DocSet::size_hint), num_docs); |
There was a problem hiding this comment.
When a Boolean OR contains another sparse OR, this estimate consumes the inner scorer's SimpleUnion::size_hint(), which returns only the largest child rather than the union. With eight disjoint 0.4%-dense terms, for example, the inner union is selected as Simple and reports 0.4% although it yields 3.2%; nesting eight such groups makes the outer branch select Simple for an approximately 25% result, so exhaustive collection recursively scans every child for each hit instead of using buffered traversal. Preserve the estimated union cardinality in SimpleUnion (or otherwise expose it here) before dispatching nested unions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a2cff6261
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if scorers.len() == 1 && !scorers[0].is::<TermScorer>() { | ||
| return SpecializedScorer::Other(scorers.into_iter().next().unwrap()); //< we checked the size beforehand | ||
| } | ||
| let estimated_hits = estimate_union(scorers.iter().map(DocSet::size_hint), num_docs); |
There was a problem hiding this comment.
When an explicitly nested OR is buffered (for example because it has more than eight clauses) and its postings are exhausted into the first 4,096-document horizon, BufferedUnionScorer::build removes all child docsets while retaining their hits in its bitsets, so BufferedUnionScorer::size_hint() reports 0. Feeding that hint into this estimate makes an outer OR of at most eight such children select SimpleUnion even when its result is dense, reintroducing up to an 8× per-hit child scan for exhaustive collectors. Preserve the buffered union's pre-refill cardinality estimate, as this change now does for SimpleUnion, before using nested hints.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c53decbdb8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| fn count(&self, reader: &SegmentReader) -> crate::Result<u32> { | ||
| let scorer = self.complex_scorer(reader, 1.0, DoNothingCombiner::default, true)?; |
There was a problem hiding this comment.
When Count evaluates a Boolean query containing only one MustNot clause, this bypasses the shortcut in scorer() (which returns EmptyScorer without constructing the child) and calls complex_scorer, which eagerly invokes the excluded weight's scorer. The query is guaranteed to match zero documents, so an expensive child such as a regex or phrase is needlessly initialized, and any child-scorer error now makes Count fail even though normal scorer-based evaluation returns zero; mirror the zero/single-clause handling before entering complex_scorer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f144d914d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let strategy = if !may_seek { | ||
| UnionStrategy::Buffered | ||
| } else { | ||
| let estimated_hits = estimate_union(scorers.iter().map(DocSet::size_hint), num_docs); |
There was a problem hiding this comment.
When an outer OR has at most eight explicitly nested Boolean queries with minimum_should_match > 1, each inner scorer is a Disjunction, whose size_hint() returns only the largest child (src/query/disjunction.rs:164-169) even though many documents can satisfy different groups of clauses. Consuming those hints here can classify a dense outer union as sparse: for example, eight inner groups built from eight disjoint pairs of 0.4%-dense terms each report 0.4% while matching 3.2%, so the outer roughly 25% union becomes a SimpleUnion and scans up to eight children for every hit. Preserve an estimated result cardinality for Disjunction or prevent these nested scorers from selecting the simple path.
Useful? React with 👍 / 👎.
Summary
Choose Boolean union traversal from clause count and estimated match density:
SimpleUnionis limited to 8 clauses, because each hit scans every child. Larger unions use buffered traversal. Eligible Block-WAND pruning remains available.SimpleUnionfor at most 128 estimated hits per 4,096 documents (3.125%). Denser unions useBufferedUnionScorer.SimpleUnionthrough the existingScoreCombiner::updateinterface, preserving child accumulation order.SimpleUnion::size_hint()usesestimate_unionwith the segment document count, so nested unions propagate their estimated cardinality to parent dispatch.Aligned scorer construction, frequency handling, single-term shortcuts, and public traits are unchanged.
Search Benchmark Game
Against upstream on Apple M4 Max: 962 queries, 5M documents in one segment, warm cache, 12 rounds.
Percentage change in latency, negative = faster. Geometric means weight queries equally. The last column sums per-query medians, giving expensive queries greater weight.
The 40 mixed intersection/union queries improve 11.85% for TOP_100 and 11.59% for TOP_100_COUNT.
Per-query examples
Upstream → this PR:
critical care nurse, COUNT: 206 -> 204 µs, approximately unchanged.aishwarya rai, COUNT: 38 -> 30 µs, 23% faster.costume designers, COUNT: 56 -> 127 µs, about 2.3× latency.cheap hotelssimilarly takes 56 -> 131 µs.critical care nurse, TOP_100_COUNT: 1,036 -> 1,869 µs, 80% slower.stone mountain, TOP_100_COUNT: 1,047 -> 1,610 µs, 54% slower.Limitations
COUNT is 3.1% slower by pure-union geomean. TOP_100_COUNT's equal-query geomean improves while its summed median latency worsens, because expensive regressions outweigh many cheap gains.
The clause limit bounds SimpleUnion's per-hit child scanning. It is a conservative guard, not a universal crossover: buffered traversal can be slower for moderately sized unions with widely spread hits.
The ordinary density estimate uses
reader.num_docs()while buffered windows span document IDs, including deleted slots. Deletion-heavy indexes, cold storage, and x86 remain unmeasured.Validation
A property test generates posting lists and constant scores, varies segment size and threshold, and compares selected-union documents/summed scores or counts with the existing buffered union. It also checks that unions above eight clauses select buffered traversal. Existing upstream tests are unchanged.
PROPTEST_CASES=1024 cargo test --lib test_union_density_and_scoring: passed.cargo test --lib: 1,344 passed, 8 ignored.cargo +nightly fmt --all,git diff --check, andcargo check --features jitexpr: passed.Implementation, validation, and benchmark analysis were assisted by an AI coding agent.