Refactor: merge _search_on_level into search_on_level#7403
Conversation
📝 WalkthroughWalkthroughReworks HNSW per-level search to be self-contained: GraphLayersBase::_search_on_level → GraphLayersBase::search_on_level and GraphLayersWithVectors::_search_on_level_with_vectors → GraphLayersWithVectors::search_on_level_with_vectors. New methods accept a level_entry and ef, allocate their own VisitedListHandle and local SearchContext(s), run the search loop internally, and return a FixedLengthPriorityQueue with results. GraphLayersBuilder is updated to call the new APIs and to drive heuristic and non‑heuristic linking using the returned nearest queue (nearest.iter_unsorted(), nearest.max(), etc.). Method signatures for the two traits changed accordingly. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.rs📄 CodeRabbit inference engine (.github/review-rules.md)
Files:
**/src/**/*.rs📄 CodeRabbit inference engine (.github/review-rules.md)
Files:
🧬 Code graph analysis (1)lib/segment/src/index/hnsw_index/graph_layers.rs (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (2)
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 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| { | ||
| let ready_list = self.ready_list.read(); | ||
| for existing_link in existing_links.iter() { | ||
| if !visited_list.check(existing_link) && ready_list[existing_link as usize] { |
There was a problem hiding this comment.
Great that you proved that this code is unreachable. We also need to remove this logic from gpu implementation in the future
There was a problem hiding this comment.
I've tried to find where this logic is implemented for GPU, but can't find it. I don't see it there:
- https://github.com/qdrant/qdrant/blob/969cc82018dd722262b93d003175b3d69961a135/lib/segment/src/index/hnsw_index/gpu/shaders/run_insert_vector.comp
- https://github.com/qdrant/qdrant/blob/969cc82018dd722262b93d003175b3d69961a135/lib/segment/src/index/hnsw_index/gpu/shaders/search_context.comp
(not that I want to remove it right now, just curious)
* Call search_on_level instead of _search_on_level * Inline _search_on_level into search_on_level * Inline _search_on_level_with_vectors into search_on_level_with_vectors
This PR merges
_search_on_levelintosearch_on_leveland removes the dead code that used it.The current
devhas two related functions:GraphLayersBase::search_on_levelandGraphLayersBase::_search_on_level.The former is high-level, and the latter is a bit more flexible as it allows reusing the
SearchContextandVisitedListHandleafter calling it.But the only place where these are reused is inside the following dead code:
qdrant/lib/segment/src/index/hnsw_index/graph_layers_builder.rs
Lines 523 to 530 in 02de826
This code is dead because
existing_linksis always empty:link_new_point/link_new_point_on_leveloradd_new_point1 is called only once per point, so it shouldn't be filled by them before.This is confirmed by codecov:
https://app.codecov.io/gh/qdrant/qdrant/blob/dev/lib%2Fsegment%2Fsrc%2Findex%2Fhnsw_index%2Fgraph_layers_builder.rs#L524
Footnotes
add_new_pointis from incremental HNSW ↩