Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if points_ids.len() < limit { | ||
| points_ids.push(link); | ||
| } |
There was a problem hiding this comment.
Does a try_for_each_link make sense here as well? To shortcut when we've filled the points ID list?
There was a problem hiding this comment.
Yes, your suggestion makes sense for consistency.
(either both limit and try_for_each_link; or neither)
- note to self
| let mut search_context = SearchContext::new(ef); | ||
| search_context.process_candidate(level_entry); | ||
|
|
||
| let limit = (self.get_m(level) as f64 * acorn_limit_factor).ceil() as usize; |
There was a problem hiding this comment.
Shouldn't we only use this limit for the 2nd hop? Not for the initial search on each node?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
📝 WalkthroughWalkthroughAdds ACORN search support and wiring across the stack: new AcornSearchParams proto and OpenAPI schema; SearchParams gains optional Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Changes are cross-cutting: proto/OpenAPI/docs, gRPC conversions, public types, core HNSW search routing, new trait methods, and tests/benches. Many files follow repetitive patterns, but critical logic in graph_layers/hnsw and new algorithm dispatch requires careful review. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
lib/segment/src/index/hnsw_index/graph_layers.rs (1)
150-244: Don’t short‑circuit the 1‑hop scan; it drops potential 2‑hop exploration.Breaking the 1‑hop iteration once
hop1_limitscored items are gathered prevents collecting remaining non‑passing neighbors for 2‑hop “hop‑over,” reducing recall. Limit should apply only to 2‑hop per 1‑hop neighbor, not the initial 1‑hop scan. This was previously raised.Apply this change: keep scanning all 1‑hop links, but cap per‑hop2 expansion only.
- // Collect 1-hop neighbors (direct neighbors) - _ = self.try_for_each_link(candidate.idx, level, |hop1| { - if hop1_visited_list.check_and_update_visited(hop1) { - return ControlFlow::Continue(()); - } - - if points_scorer.filters().check_vector(hop1) { - to_score.push(hop1); - if to_score.len() >= hop1_limit { - return ControlFlow::Break(()); - } - } else { - to_explore.push(hop1); - } - ControlFlow::Continue(()) - }); + // Collect 1-hop neighbors (direct neighbors): don't early-break. + let filters = points_scorer.filters(); + self.for_each_link(candidate.idx, level, |hop1| { + if hop1_visited_list.check_and_update_visited(hop1) { + return; + } + if filters.check_vector(hop1) { + to_score.push(hop1); + } else { + to_explore.push(hop1); + } + });Optional: if you still want to bound 1‑hop scoring cost, gate only the push, not the iteration:
if filters.check_vector(hop1) && to_score.len() < hop1_limit { to_score.push(hop1); }
🧹 Nitpick comments (8)
lib/segment/src/index/hnsw_index/point_scorer.rs (1)
226-239: Add documentation for the new public method.The implementation is correct and provides the unfiltered scoring capability needed for ACORN. However, as a public method, it should include a doc comment explaining its purpose, parameters, and return value—especially noting that it bypasses all filters.
Apply this diff to add documentation:
+ /// Scores the given slice of point IDs without applying filters. + /// + /// Unlike [`score_points`], this method does not check whether points satisfy + /// the filter context or are marked as deleted. The caller is responsible for + /// ensuring the points are valid. + /// + /// # Arguments + /// + /// * `point_ids` - Slice of point IDs to score. + /// + /// # Returns + /// + /// An iterator of scored point offsets. pub fn score_points_unfiltered( &mut self, point_ids: &[PointOffsetType], ) -> impl Iterator<Item = ScoredPointOffset> {docs/redoc/master/openapi.json (1)
9172-9184: Document themax_selectivitydefault in the schemaThe description calls out a default of 0.4, but the JSON schema doesn’t expose it. Adding the
defaultattribute keeps ReDoc/SDK consumers in sync with the implementation and avoids clients needing to hard-code the value themselves."max_selectivity": { "description": "Maximum selectivity of filters to enable ACORN.\n\nIf estimated filters selectivity is higher than this value, ACORN will not be used. Selectivity is estimated as: `estimated number of points satisfying the filters / total number of points`.\n\n0.0 for never, 1.0 for always. Default is 0.4.", "type": "number", "format": "double", "maximum": 1, "minimum": 0, - "nullable": true + "nullable": true, + "default": 0.4 }lib/segment/src/types.rs (1)
531-556: Default semantics vs. docs mismatch for max_selectivityDocs say “Default is 0.4”, but derive(Default) yields
enable = falseandmax_selectivity = None. This can confuse users and serializers. Consider making the default explicit or updating the doc to “unspecified uses internal default 0.4”.Two options:
- Preferred (explicit default when Acorn is present):
-#[derive( - Debug, Deserialize, Serialize, JsonSchema, Validate, Clone, Copy, PartialEq, Default, Hash, -)] +#[derive( + Debug, Deserialize, Serialize, JsonSchema, Validate, Clone, Copy, PartialEq, Hash, +)] pub struct AcornSearchParams { @@ - #[serde(default)] - #[serde(skip_serializing_if = "Option::is_none")] - #[validate(range(min = 0.0, max = 1.0))] - pub max_selectivity: Option<OrderedFloat<f64>>, + #[serde(default = "acorn_max_selectivity_default")] + #[validate(range(min = 0.0, max = 1.0))] + pub max_selectivity: OrderedFloat<f64>, } + +fn acorn_max_selectivity_default() -> OrderedFloat<f64> { + OrderedFloat(ACORN_MAX_SELECTIVITY_DEFAULT) +} + +impl Default for AcornSearchParams { + fn default() -> Self { + Self { enable: false, max_selectivity: acorn_max_selectivity_default() } + } +}
- Or, keep Option but fix docs and use the constant when
Noneat call sites.Pick one and align GRPC/REST docs accordingly.
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (1)
23-25: Redundant test-only import
SearchAlgorithmis imported again inside thetestsmodule below. You can drop this top-level#[cfg(test)]import to avoid duplication.lib/segment/src/index/hnsw_index/graph_layers.rs (3)
80-85: Provide a default for SearchAlgorithm (Hnsw) to reduce call‑site churn.Add Default to ease migration and allow API wrappers to use
..Default::default()patterns.#[derive(Debug, Clone, Copy, Eq, PartialEq)] pub enum SearchAlgorithm { Hnsw, Acorn, } + +impl Default for SearchAlgorithm { + fn default() -> Self { + SearchAlgorithm::Hnsw + } +}
184-186: Consider extracting the “16” into a named constant.Magic number; make the reservation heuristic self‑documenting.
- let mut to_score = Vec::with_capacity(hop1_limit * hop2_limit.min(16)); - let mut to_explore = Vec::with_capacity(hop1_limit * hop2_limit.min(16)); + const ACORN_RESERVE_HOP2_MAX: usize = 16; + let reserve = hop1_limit * hop2_limit.min(ACORN_RESERVE_HOP2_MAX); + let mut to_score = Vec::with_capacity(reserve); + let mut to_explore = Vec::with_capacity(reserve);
741-749: Tests: update done; add ACORN path coverage.Add a focused test that:
- constructs a small graph where 1‑hop neighbors fail the filter and 2‑hop pass;
- asserts HNSW misses but ACORN returns expected points.
I can draft this test if helpful.
lib/segment/src/index/hnsw_index/hnsw.rs (1)
979-1005: ACORN algorithm selection logic looks sound.
- Gated by
enable,m0 != 0, and presence of filter.- Uses adjusted cardinality and available vectors to compute selectivity.
- Falls back to HNSW when not under threshold.
Minor: cache
filter.is_some()into a local to slightly simplify the condition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/grpc/docs.md(3 hunks)docs/redoc/master/openapi.json(2 hunks)lib/api/build.rs(1 hunks)lib/api/src/grpc/conversions.rs(4 hunks)lib/api/src/grpc/proto/points.proto(2 hunks)lib/api/src/grpc/qdrant.rs(2 hunks)lib/edge/python/src/search.rs(1 hunks)lib/segment/benches/hnsw_build_asymptotic.rs(3 hunks)lib/segment/benches/hnsw_search_graph.rs(3 hunks)lib/segment/src/index/hnsw_index/gpu/mod.rs(2 hunks)lib/segment/src/index/hnsw_index/graph_layers.rs(10 hunks)lib/segment/src/index/hnsw_index/graph_layers_builder.rs(5 hunks)lib/segment/src/index/hnsw_index/hnsw.rs(5 hunks)lib/segment/src/index/hnsw_index/point_scorer.rs(1 hunks)lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs(2 hunks)lib/segment/src/types.rs(2 hunks)lib/segment/tests/integration/segment_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust code
Do not use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice in new code; use bytemuck or zerocopy instead
Files:
lib/api/build.rslib/segment/benches/hnsw_search_graph.rslib/segment/benches/hnsw_build_asymptotic.rslib/segment/src/index/hnsw_index/point_scorer.rslib/segment/src/index/hnsw_index/graph_layers_builder.rslib/segment/src/index/hnsw_index/graph_layers.rslib/segment/tests/integration/segment_tests.rslib/segment/src/types.rslib/api/src/grpc/conversions.rslib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rslib/segment/src/index/hnsw_index/hnsw.rslib/api/src/grpc/qdrant.rslib/edge/python/src/search.rslib/segment/src/index/hnsw_index/gpu/mod.rs
**/{tests,benches}/**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
Using .unwrap() and panic!() in tests and benchmarks is acceptable and should not be flagged
Files:
lib/segment/benches/hnsw_search_graph.rslib/segment/benches/hnsw_build_asymptotic.rslib/segment/tests/integration/segment_tests.rslib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/src/**/*.rs: Prefer exhaustive match arms over a catch-all _ arm to avoid missing new enum variants (except in tests/benchmarks or when provably safe)
Prefer explicit field ignoring with : _ over .. in struct patterns (except in tests/benchmarks or when provably safe)
Files:
lib/segment/src/index/hnsw_index/point_scorer.rslib/segment/src/index/hnsw_index/graph_layers_builder.rslib/segment/src/index/hnsw_index/graph_layers.rslib/segment/src/types.rslib/api/src/grpc/conversions.rslib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rslib/segment/src/index/hnsw_index/hnsw.rslib/api/src/grpc/qdrant.rslib/edge/python/src/search.rslib/segment/src/index/hnsw_index/gpu/mod.rs
🧠 Learnings (1)
📚 Learning: 2025-05-30T15:26:14.488Z
Learnt from: generall
PR: qdrant/qdrant#6479
File: lib/segment/src/index/hnsw_index/graph_layers_builder.rs:137-182
Timestamp: 2025-05-30T15:26:14.488Z
Learning: In the subgraph_connectivity method in graph_layers_builder.rs, the previous_visited_points vector and visited BitVec entry point marking are automatically re-initialized at the start of each retry loop iteration, so manual clearing is not needed.
Applied to files:
lib/segment/src/index/hnsw_index/graph_layers_builder.rs
🧬 Code graph analysis (8)
lib/segment/benches/hnsw_build_asymptotic.rs (1)
lib/segment/src/fixtures/index_fixtures.rs (1)
scorer(87-97)
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (2)
lib/segment/src/index/hnsw_index/graph_layers.rs (2)
try_for_each_link(93-100)try_for_each_link(466-476)lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
links(215-246)
lib/segment/src/index/hnsw_index/graph_layers.rs (2)
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (2)
for_each_link(58-69)try_for_each_link(71-88)lib/segment/src/common/operation_error.rs (1)
check_process_stopped(248-253)
lib/segment/src/types.rs (1)
lib/api/src/grpc/validate.rs (4)
validate(14-14)validate(19-21)validate(29-31)validate(39-41)
lib/api/src/grpc/conversions.rs (1)
lib/segment/src/types.rs (16)
from(140-142)from(200-202)from(256-261)from(265-270)from(889-891)from(895-897)from(901-903)from(938-947)from(951-954)from(981-990)from(1002-1005)from(1242-1286)from(1805-1807)from(1811-1813)from(1817-1819)from(1832-1837)
lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs (1)
lib/segment/src/fixtures/index_fixtures.rs (1)
scorer(87-97)
lib/segment/src/index/hnsw_index/hnsw.rs (3)
lib/segment/src/vector_storage/vector_storage_base.rs (17)
v(660-660)v(662-662)v(664-664)v(665-665)v(667-667)v(669-669)v(670-670)v(671-671)v(672-672)v(673-673)v(674-674)v(675-675)v(676-676)v(677-677)v(678-678)v(944-944)available_vector_count(82-85)lib/segment/src/index/query_estimator.rs (1)
adjust_to_available_vectors(25-65)lib/segment/src/index/hnsw_index/graph_layers.rs (1)
search_with_vectors(564-596)
lib/api/src/grpc/qdrant.rs (1)
lib/api/src/grpc/validate.rs (8)
validate(14-14)validate(19-21)validate(29-31)validate(39-41)validate(49-54)validate(58-64)validate(68-74)validate(78-85)
🪛 markdownlint-cli2 (0.18.1)
docs/grpc/docs.md
140-140: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
2449-2449: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
⏰ 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)
- GitHub Check: test-consensus-compose
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: test-consistency
- GitHub Check: integration-tests
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: integration-tests-consensus
🔇 Additional comments (26)
lib/api/build.rs (1)
200-200: LGTM! Validation constraint is appropriate.The range constraint
[0.0, 1.0]formax_selectivityis correct for a selectivity parameter. The placement within the Points service validation is also appropriate.lib/segment/src/index/hnsw_index/point_scorer.rs (1)
223-224: Clean refactoring that maintains existing behavior.Delegating to
score_points_unfilteredseparates filtering concerns from scoring logic, which is beneficial for supporting ACORN's unfiltered scoring needs.lib/segment/src/types.rs (1)
585-590: SearchParams.acorn field addition looks goodOptional nested params with serde default and nested validation is consistent with existing patterns. LGTM.
lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs (2)
10-10: Correct import for new enumBringing
SearchAlgorithmalongsideGraphLayersBaseis required for the new signature. LGTM.
99-106: API migration to.search(..., SearchAlgorithm::Hnsw, ...)Signature adaptation is correct and preserves previous behavior.
lib/segment/benches/hnsw_build_asymptotic.rs (3)
11-11: Bench imports updated appropriatelyImporting
SearchAlgorithmfor the new search API is correct.
42-49: Bench uses Hnsw path explicitlyThe added
SearchAlgorithm::Hnswin.searchmatches intent and keeps results comparable across runs.
68-75: Second bench call updated consistentlyConsistent with prior change; no issues.
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (3)
4-4: ControlFlow importNeeded for the new fallible iterator-style method. LGTM.
764-771: Tests: updated search signaturePassing
SearchAlgorithm::Hnswis correct here.
877-884: Tests: second search call updated consistentlyLooks good.
lib/segment/src/index/hnsw_index/gpu/mod.rs (3)
106-106: Tests import new enum and typeBringing in
GraphLayersandSearchAlgorithmfor test comparisons is correct.
217-224: GPU path: explicit Hnsw selectionAPI migration correct; preserves baseline for GPU-vs-CPU quality checks.
231-237: CPU ref path: consistent updateMirrors GPU change; no issues.
lib/segment/src/index/hnsw_index/graph_layers.rs (4)
93-101: try_for_each_link API looks good.Consistent with builder’s variant; enables short‑circuiting where needed.
466-477: GraphLayers.try_for_each_link delegation is correct.Properly forwards to links container; aligns with builder’s implementation.
1-26: Docs/imports LGTM.Helpful module docs; ControlFlow import and doc additions improve readability.
Also applies to: 29-29, 105-108, 276-279, 333-336, 417-420
530-561: Review comment is incorrect—GraphLayers::search is already properly updated.The code is correct. GraphLayers::search is an internal method called only once in the codebase (line 1067 of hnsw.rs), and that call site already passes SearchAlgorithm as the 3rd argument with the correct signature. The verification script in the review caught false positives—the
.search()calls found were from other index types (VectorIndex wrappers, SparseVectorIndex, etc.), not GraphLayers. There are no "outdated calls" to fix. The API change is properly encapsulated within the Hnsw implementation.Likely an incorrect or invalid review comment.
lib/api/src/grpc/conversions.rs (4)
11-11: Import for OrderedFloat is appropriate.Required for wrapping/unwrapping
max_selectivity. No issues.
47-58: qdrant proto import extended (AcornSearchParams).Looks consistent with added conversions below.
790-801: Bi‑directional AcornSearchParams conversions are correct and symmetric.
enabledefaults to false when unset on gRPC side.max_selectivityuses OrderedFloat wrapper safely both ways.Consider adding a small round‑trip test (gRPC -> internal -> gRPC) for
AcornSearchParamsto guard serialization defaults.Also applies to: 803-814
823-832: SearchParams now carriesacornacross the boundary.Mapping uses explicit
Type::from, matches our guidelines. Defaulting of other fields preserved.Please ensure REST conversions (if any) also propagate
acornconsistently; if not applicable, ignore.Also applies to: 842-851
lib/segment/src/index/hnsw_index/hnsw.rs (4)
46-48: Importing SearchAlgorithm is correct.Needed for routing to HNSW vs ACORN.
66-68: ACORN constants and types import OK.
ACORN_MAX_SELECTIVITY_DEFAULTandSearchParamsare used below as expected.
1008-1012: Early exit for with‑vectors path when using ACORN.Clear behavior until ACORN-on-link-vectors is implemented.
Optionally add a TODO linking to an issue if ACORN with-vectors is planned, to avoid regressions later.
1069-1071: Pass-through of algorithm into graph.search is correct.API change is plumbed properly.
| fn try_for_each_link<F>( | ||
| &self, | ||
| point_id: PointOffsetType, | ||
| level: usize, | ||
| mut f: F, | ||
| ) -> ControlFlow<(), ()> | ||
| where | ||
| F: FnMut(PointOffsetType) -> ControlFlow<(), ()>, | ||
| { | ||
| let links = self.links_layers[point_id as usize][level].read(); | ||
| let ready_list = self.ready_list.read(); | ||
| for link in links.iter() { | ||
| if ready_list[link as usize] { | ||
| f(link)?; | ||
| } | ||
| } | ||
| ControlFlow::Continue(()) | ||
| } |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| let hop1_limit = self.get_m(level); | ||
| let hop2_limit = self.get_m(level); | ||
| debug_assert_ne!(self.get_m(level), 0); // See `FilteredBytesScorer::score_points` | ||
|
|
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| let acorn_enabled = params | ||
| .and_then(|params| params.acorn) | ||
| .is_some_and(|acorn| acorn.enable); | ||
| let acorn_max_selectivity = params | ||
| .and_then(|params| params.acorn) | ||
| .and_then(|acorn| acorn.max_selectivity) | ||
| .map_or(ACORN_MAX_SELECTIVITY_DEFAULT, |v| *v); | ||
|
|
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| /// Default value for [`AcornSearchParams::max_selectivity`]. | ||
| /// | ||
| /// After change, update docs for GRPC and REST API. | ||
| pub const ACORN_MAX_SELECTIVITY_DEFAULT: f64 = 0.4; | ||
|
|
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lib/segment/src/index/hnsw_index/hnsw.rs (1)
957-1005: Critical: Fix OrderedFloat access; verify selectivity threshold semantics.Line 963 attempts to dereference
OrderedFloatwith*v, which will not compile becauseOrderedFloatdoes not implementDeref.Apply this fix:
let acorn_max_selectivity = params .and_then(|params| params.acorn) .and_then(|acorn| acorn.max_selectivity) - .map_or(ACORN_MAX_SELECTIVITY_DEFAULT, |v| *v); + .map_or(ACORN_MAX_SELECTIVITY_DEFAULT, |v| v.0);
Additionally, verify the ACORN selectivity calculation aligns with the paper's definition. The current implementation computes
query_cardinality.exp / available_vector_countand compares againstacorn_max_selectivity. Ensure this threshold semantics matches the intended behavior described in the ACORN paper (e.g., whether selectivity should represent the fraction of vectors passing the filter).lib/segment/src/index/hnsw_index/graph_layers.rs (1)
154-243: Minor: Misleading debug_assert comment; verify 2-hop budget semantics.Line 182's comment references
FilteredBytesScorer::score_points, but this function callsFilteredScorer::score_points_unfilteredat line 238. Update the comment to clarify the actual invariant.Suggested fix:
- debug_assert_ne!(self.get_m(level), 0); // See `FilteredBytesScorer::score_points` + // ACORN uses M to compute per-hop budgets; M must not be zero. + debug_assert_ne!(self.get_m(level), 0);
Additionally, verify the 2-hop limit semantics. At line 218,
total_limit = to_score.len() + hop2_limitmeans each filtered 1-hop node can contribute up tohop2_limitadditional 2-hop neighbors. Confirm this matches the ACORN-1 paper's definition of "limit factor" to avoid scoring too many or too few candidates.Run the following script to check how
get_mis used across the codebase and ensure the assertion holds:#!/bin/bash # Verify that get_m(level) is always non-zero when used in ACORN scoring rg -nP '\bget_m\(' --type=rs -A2 -B2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/grpc/docs.md(3 hunks)docs/redoc/master/openapi.json(2 hunks)lib/api/build.rs(2 hunks)lib/api/src/grpc/proto/points.proto(2 hunks)lib/api/src/grpc/qdrant.rs(2 hunks)lib/segment/src/index/hnsw_index/graph_layers.rs(10 hunks)lib/segment/src/index/hnsw_index/hnsw.rs(5 hunks)lib/segment/src/types.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/segment/src/types.rs
- docs/redoc/master/openapi.json
- lib/api/build.rs
- lib/api/src/grpc/proto/points.proto
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust code
Do not use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice in new code; use bytemuck or zerocopy instead
Files:
lib/segment/src/index/hnsw_index/graph_layers.rslib/api/src/grpc/qdrant.rslib/segment/src/index/hnsw_index/hnsw.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/src/**/*.rs: Prefer exhaustive match arms over a catch-all _ arm to avoid missing new enum variants (except in tests/benchmarks or when provably safe)
Prefer explicit field ignoring with : _ over .. in struct patterns (except in tests/benchmarks or when provably safe)
Files:
lib/segment/src/index/hnsw_index/graph_layers.rslib/api/src/grpc/qdrant.rslib/segment/src/index/hnsw_index/hnsw.rs
🧬 Code graph analysis (3)
lib/segment/src/index/hnsw_index/graph_layers.rs (1)
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (3)
get_visited_list_from_pool(54-56)for_each_link(58-69)try_for_each_link(71-88)
lib/api/src/grpc/qdrant.rs (1)
lib/api/src/grpc/validate.rs (8)
validate(14-14)validate(19-21)validate(29-31)validate(39-41)validate(49-54)validate(58-64)validate(68-74)validate(78-85)
lib/segment/src/index/hnsw_index/hnsw.rs (3)
lib/segment/src/vector_storage/vector_storage_base.rs (17)
v(660-660)v(662-662)v(664-664)v(665-665)v(667-667)v(669-669)v(670-670)v(671-671)v(672-672)v(673-673)v(674-674)v(675-675)v(676-676)v(677-677)v(678-678)v(944-944)available_vector_count(82-85)lib/segment/src/index/query_estimator.rs (1)
adjust_to_available_vectors(25-65)lib/segment/src/index/hnsw_index/graph_layers.rs (1)
search_with_vectors(564-596)
🪛 markdownlint-cli2 (0.18.1)
docs/grpc/docs.md
140-140: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
2449-2449: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
⏰ 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)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: lint
- GitHub Check: test-consistency
- GitHub Check: e2e-tests
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-consensus-compose
- GitHub Check: integration-tests-consensus
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: integration-tests
- GitHub Check: rust-tests (ubuntu-latest)
🔇 Additional comments (14)
docs/grpc/docs.md (2)
2441-2462: Verify value ranges for max_selectivity.The documentation states that
max_selectivityshould be "0.0 for never, 1.0 for always" with a default of 0.4. Confirm that the implementation enforces these bounds (0.0 ≤ value ≤ 1.0) to prevent invalid configurations. If validation exists upstream, consider adding a validation note to the docs for clarity.
2441-2462: Documentation accurately reflects AcornSearchParams API.The proto documentation correctly documents the new
AcornSearchParamsmessage and its integration intoSearchParams. Field names, types, and descriptions align with the implementation. The design has evolved from the earlieracorn_limit_factorparameter proposal to a more granular approach with separateenableandmax_selectivitycontrols, which is consistent with the PR objective that noted the design may change.Also applies to: 4764-4764
lib/api/src/grpc/qdrant.rs (2)
4853-4871: Nice validation bounds onmax_selectivity. This cleanly matches the ACORN paper’s 0–1 contract and keeps bad configs out at the API edge.
4893-4896: Nested validation hook is exactly what we needed. With#[validate(nested)]onacorn, the top-level validator now respects the new struct, so we won’t regress on input sanitization.lib/segment/src/index/hnsw_index/hnsw.rs (4)
46-48: LGTM: Import changes align with ACORN integration.The new imports for
SearchAlgorithmandGraphLayersWithVectorsare correctly positioned and support the algorithm selection logic added downstream.
66-67: LGTM: ACORN-related imports added.The imports for
ACORN_MAX_SELECTIVITY_DEFAULTand the updatedSearchParamstype correctly support the new ACORN threshold logic.
1007-1012: LGTM: Early return for unimplemented ACORN-with-vectors path is appropriate.The comment clearly documents that ACORN is not yet implemented for graph-with-vectors, and the early return prevents silent failures.
1067-1074: LGTM: Algorithm propagation to graph search is correct.The selected
algorithmis correctly passed to the underlyinggraph.search()call, enabling the dispatch logic inGraphLayers.lib/segment/src/index/hnsw_index/graph_layers.rs (6)
1-26: LGTM: Module documentation clearly describes search variants.The new module-level documentation provides a helpful overview of the five search functions and their differences, improving code navigability.
29-29: LGTM: ControlFlow import supports early-exit iteration.The
ControlFlowimport is necessary for the newtry_for_each_linkAPI.
80-84: LGTM: SearchAlgorithm enum is well-defined.The public enum with
HnswandAcornvariants provides a clear, type-safe way to select the search strategy. DerivingCopy,Eq, andPartialEqis appropriate for a simple discriminant.
93-100: LGTM: try_for_each_link enables early-exit iteration.The new trait method allows callers to short-circuit link iteration, which is essential for the ACORN algorithm's per-hop budgets.
466-476: LGTM: try_for_each_link delegation is correct.The implementation correctly delegates to
self.links.links(point_id, level).try_for_each(f), propagating theControlFlowresult.
530-561: LGTM: Algorithm dispatch logic is sound.The
searchfunction correctly accepts the newalgorithmparameter and dispatches to eithersearch_on_level(Hnsw) orsearch_on_level_acorn(Acorn) using exhaustive matching. Themax(ef, top)normalization at line 551 is appropriate.
| - [NullValue](#qdrant-NullValue) | ||
|
|
||
| - [points.proto](#points-proto) | ||
| - [AcornSearchParams](#qdrant-AcornSearchParams) |
There was a problem hiding this comment.
Fix indentation in TOC entry.
The unordered list item should use 2-space indentation per Markdown conventions, not 4 spaces.
- [AcornSearchParams](#qdrant-AcornSearchParams)
- - [BatchResult](#qdrant-BatchResult)
+ - [BatchResult](#qdrant-BatchResult)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
140-140: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents
In docs/grpc/docs.md around line 140, the TOC entry "-
[AcornSearchParams](#qdrant-AcornSearchParams)" is indented with 4 spaces;
change it to use 2-space indentation for the unordered list item so it follows
the Markdown convention (replace the 4-space indent with 2 spaces).
|
|
||
| | Field | Type | Label | Description | | ||
| | ----- | ---- | ----- | ----------- | | ||
| | enable | [bool](#bool) | optional | If true, then ACORN may be used for the HNSW search based on filters selectivity. |
There was a problem hiding this comment.
Add missing trailing pipe to table header.
Markdown table formatting requires consistent leading and trailing pipes. Line 2449 is missing the trailing pipe.
~| ----- | ---- | ----- | ----------- |
~| ----- | ---- | ----- | ----------- |Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
2449-2449: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
🤖 Prompt for AI Agents
In docs/grpc/docs.md around line 2449, the markdown table row for the "enable"
field is missing a trailing pipe; edit the line to add the closing "|" so the
table cells are properly delimited and the table renders correctly.
* GraphLayersBase::try_for_each_link * Add FilteredScorer::score_points_unfiltered * Add GraphLayersBase::search_on_level_acorn() * Add SearchParams::acorn API parameter * Integrate ACORN to HNSW search * Add doc * review fixes * Misc fixes --------- Co-authored-by: generall <andrey@vasnetsov.com>
This PR adds support for ACORN-1 as described in the ACORN paper.
This is a variation of HNSW search that "hops" over neighbors that do not pass the filter.
The index structure remains the same.
The new search is controlled/enabled via a new search parameter
acorn_limit_factor, e.g.:When the new parameter is
nullor not present, the search behaves the same as before (without ACORN-1).The design/name of a new parameter is subject to change later as we perform more experiments (we might add more knobs or change it to a boolean flag).
Also, since the number of "search on level" functions reached five, this PR adds a module doc comment describing their differences.
Benchmarks
Benchmark results on random vectors:
dim=128,m=16,payload_m=0,ef_search=256,num_points=500_000Without ACORN:
With ACORN: