feat(index): parallel segment search with rayon - #115
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughParallelizes BM25 document scoring by using Rayon to compute (score, doc_id) tuples in parallel over doc IDs, then sequentially rehydrates document sources for aggregation, sorting, pagination, and hit construction; adds ADR, workspace/crate dependencies, and Criterion benchmark suite. ChangesParallel Segment Search Using Rayon
Sequence Diagram(s)sequenceDiagram
participant Client
participant Search as IndexHandle::search
participant Parallel as Rayon ParallelIterator
participant Storage as Document Store
Client->>Search: search(query)
Search->>Parallel: par_iter(doc_ids) filter and score
Parallel->>Parallel: BM25 scoring
Parallel->>Search: scores and doc_ids
Search->>Storage: Sequential source lookup
Storage->>Search: sources with scores
Search->>Search: Aggregations, sort, search_after
Search->>Client: SearchResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Parallelizes the hot loop in search() that scores documents against the query. Only doc_id (String, Send+Sync) crosses thread boundaries; source lookups happen inside each parallel task via &self, avoiding the Send+Sync requirement on IndexDocument (whose serde_json::Value field contains RefCell). The scoring phase now uses into_par_iter() on doc_ids, filtering expired docs and MLT exclusions in parallel, then lookups source sequentially after scoring (O(hits) not O(all_docs)).
poyrazK
force-pushed
the
feature/autocomplete-suggest
branch
from
June 11, 2026 18:09
f4bc042 to
2f4361f
Compare
Benchmarks for search() across MatchAll, term, multi_match, and range queries at 1k and 10k document counts. Run with: cargo bench -p cloudsearch-index
poyrazK
added a commit
that referenced
this pull request
Jun 11, 2026
poyrazK
added a commit
that referenced
this pull request
Jun 11, 2026
poyrazK
force-pushed
the
feature/autocomplete-suggest
branch
from
June 11, 2026 19:21
ccd38de to
04689be
Compare
poyrazK
added a commit
that referenced
this pull request
Jun 11, 2026
poyrazK
force-pushed
the
feature/autocomplete-suggest
branch
from
June 11, 2026 19:23
04689be to
b3dcad0
Compare
poyrazK
added a commit
that referenced
this pull request
Jun 11, 2026
poyrazK
force-pushed
the
feature/autocomplete-suggest
branch
from
June 11, 2026 19:31
b3dcad0 to
4f164da
Compare
poyrazK
added a commit
that referenced
this pull request
Jun 11, 2026
poyrazK
force-pushed
the
feature/autocomplete-suggest
branch
from
June 11, 2026 19:38
4f164da to
6ecd339
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rust/crates/cloudsearch-index/src/lib.rs (1)
1029-1044: ⚡ Quick winAvoid full
sourceclones inside sort comparator.Line 1029-1044 clones full documents for each comparison; compare on borrowed sort field values and
doc_idinstead to keep sort overhead low.Suggested refactor
- scored.sort_by(|(_, l_id, l_src), (_, r_id, r_src)| { - let lh = SearchHit { - id: l_id.clone(), - source: l_src.clone(), - score: None, - highlight: None, - sort_values: None, - }; - let rh = SearchHit { - id: r_id.clone(), - source: r_src.clone(), - score: None, - highlight: None, - sort_values: None, - }; - compare_hits(&lh, &rh, sort) - }); + scored.sort_by(|(_, l_id, l_src), (_, r_id, r_src)| { + let l_val = l_src.get(&sort.field).and_then(comparable_value); + let r_val = r_src.get(&sort.field).and_then(comparable_value); + + let ordering = match (l_val, r_val) { + (Some(l), Some(r)) => compare_sort_values(&l, &r, sort), + (None, Some(_)) => std::cmp::Ordering::Greater, + (Some(_), None) => std::cmp::Ordering::Less, + (None, None) => l_id.cmp(r_id), + }; + + if ordering == std::cmp::Ordering::Equal { + l_id.cmp(r_id) + } else { + ordering + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/cloudsearch-index/src/lib.rs` around lines 1029 - 1044, The comparator passed to scored.sort_by is cloning entire SearchHit.source for every comparison (in the closure that builds lh/rh and calls compare_hits), which is expensive; change the comparator to compare only the doc_id and the specific sort-field values by borrowing them instead of cloning the whole source. Concretely, in the scored.sort_by closure avoid constructing full SearchHit with cloned source—either extract the needed sort values and ids from l_src/r_src and call a new helper like compare_hits_by_values(&l_id, &r_id, l_sort_vals, r_sort_vals, sort) or change compare_hits to accept borrowed inputs (e.g., &str id and &Value or &Vec<Value> sort_values) so you only clone the doc_id when necessary and borrow sort fields from l_src/r_src. Ensure references to the existing symbols: scored.sort_by closure, SearchHit construction site, compare_hits (or a new compare_hits_by_values), and the sort variable are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0005-parallel-segment-search.md`:
- Around line 13-14: Replace the incorrect rationale in the ADR: remove the
claim that serde_json::Value “uses RefCell internally” and is not Send/Sync, and
instead state that serde_json::Value does implement Send + Sync; correct the
Rayon bound by noting that into_par_iter() on a Vec<T> requires T: Send (not T:
Send + Sync). Update the discussion around IndexDocument::source to explain the
actual constraint driving the design (e.g., that IndexDocument::source is
intentionally not moved across threads in current architecture or that
cloning/moving the Value is avoided for performance/ownership reasons), and
ensure the text aligns with the later statement that serde_json::Value “never
crosses thread boundaries.”
In `@rust/crates/cloudsearch-index/benches/search.rs`:
- Around line 58-67: The benchmark setup currently ignores Results from async
calls (e.g., catalog.initialize(), catalog.create_index(...), and the later
indexing calls), allowing silent failures; change these to propagate or fail
fast by handling the Result (use the ? operator if the surrounding function
returns Result, or call .expect("...")/.unwrap_or_else(|e| panic!(...)) with a
clear message) so any error from catalog.initialize(),
catalog.create_index(...), and the subsequent indexing calls is surfaced
immediately and aborts the benchmark.
---
Nitpick comments:
In `@rust/crates/cloudsearch-index/src/lib.rs`:
- Around line 1029-1044: The comparator passed to scored.sort_by is cloning
entire SearchHit.source for every comparison (in the closure that builds lh/rh
and calls compare_hits), which is expensive; change the comparator to compare
only the doc_id and the specific sort-field values by borrowing them instead of
cloning the whole source. Concretely, in the scored.sort_by closure avoid
constructing full SearchHit with cloned source—either extract the needed sort
values and ids from l_src/r_src and call a new helper like
compare_hits_by_values(&l_id, &r_id, l_sort_vals, r_sort_vals, sort) or change
compare_hits to accept borrowed inputs (e.g., &str id and &Value or &Vec<Value>
sort_values) so you only clone the doc_id when necessary and borrow sort fields
from l_src/r_src. Ensure references to the existing symbols: scored.sort_by
closure, SearchHit construction site, compare_hits (or a new
compare_hits_by_values), and the sort variable are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a68d3fee-9724-4a58-b2fa-bb23b7915587
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
docs/adr/0005-parallel-segment-search.mddocs/roadmap.mdrust/Cargo.tomlrust/crates/cloudsearch-index/Cargo.tomlrust/crates/cloudsearch-index/benches/search.rsrust/crates/cloudsearch-index/src/lib.rs
poyrazK
added a commit
that referenced
this pull request
Jun 11, 2026
poyrazK
force-pushed
the
feature/autocomplete-suggest
branch
from
June 11, 2026 19:45
6ecd339 to
0a363b8
Compare
poyrazK
force-pushed
the
feature/autocomplete-suggest
branch
from
June 11, 2026 19:51
0a363b8 to
bb47405
Compare
…pagation, sort clone overhead - ADR 0005: correct serde_json::Value Send+Sync claim; explain the actual design constraint is avoiding cloning overhead, not a Send+Sync blocker - benches/search.rs: propagate all async Result errors with expect() so catalog init/index/flush failures abort the benchmark immediately - lib.rs: avoid cloning full SearchHit.source in sort comparator; extract sort field values directly from l_src/r_src (serde_json::Value::get borrows)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Summary by CodeRabbit
New Features
Documentation
Tests