mirror of
https://github.com/qdrant/qdrant.git
synced 2026-08-03 16:40:54 -05:00
8de9c11a5d
* refactor(index): drop PayloadIndex: PayloadIndexRead super-trait `PayloadIndex` now only declares the mutating surface; reads live on the sibling `PayloadIndexRead` trait. The previous super-trait relationship forced any type that implemented `PayloadIndex` to also implement `PayloadIndexRead`, blocking a future `PayloadIndexRead`-only view that doesn't (and shouldn't) own the writable index machinery. No behavioural change. Audit before committing showed no generic bound site on `PayloadIndex` exists in the workspace, and every caller that uses read methods already imports `PayloadIndexRead` explicitly (the trait was already used as a generic bound on `SegmentReadView`'s `TPayloadIndex` parameter and on `iter_filtered_points`). The full workspace builds clean and all segment / storage / collection tests pass without any consumer update. Doc comment on `PayloadIndex` updated to point readers at `PayloadIndexRead` for the read surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(index): introduce StructPayloadIndexReadView<P, I, V> (#8970) Move the read surface of `StructPayloadIndex` onto a new borrowed view struct generic over `<P: PayloadStorageRead, I: IdTrackerRead, V: VectorStorageRead>`. The view holds exactly the fields that `PayloadIndexRead` requires -- no more, no less: pub struct StructPayloadIndexReadView<'a, P, I, V> { payload: &'a Arc<AtomicRefCell<P>>, id_tracker: &'a I, vector_storages: &'a HashMap<VectorNameBuf, Arc<AtomicRefCell<V>>>, field_indexes: &'a IndexesMap, config: &'a PayloadConfig, visited_pool: &'a VisitedPool, } `StructPayloadIndex` now exposes a `with_view(|v| ...)` accessor that borrows `id_tracker` once at the top and constructs the view for the closure scope. All read-method bodies move onto the view, which is the sole `PayloadIndexRead` implementor for this index. Why three generics ================== - `P: PayloadStorageRead` -- already generic via PR #8968. - `I: IdTrackerRead` -- direct method calls; held as `&I` (not `&Arc<AtomicRefCell<I>>`) because the cell is collapsed at the `with_view` boundary, saving a per-method `borrow()` atomic op. `dyn IdTrackerRead` does not satisfy `I: IdTrackerRead` bounds in Rust without an explicit blanket impl, so generic is the only consistent option here. - `V: VectorStorageRead` -- the only access site is `available_vector_count()` for the `HasVector` cardinality branch (`condition_cardinality` in `read_view/filtering.rs`). Why `payload` keeps the `Arc` ============================= `PayloadProvider<P>::new(...)` (introduced in PR #8968) takes `Arc<AtomicRefCell<P>>` so that the returned `FormulaScorer<'q>` / `Box<dyn FilterContext + 'a>` can outlive the caller frame. The view therefore holds `&'a Arc<AtomicRefCell<P>>` (asymmetric vs the bare `&I` for `id_tracker`). Switching to a borrow-based provider would require reworking `formula_scorer` / `filter_context` to callback style; deferred to a follow-up if needed. What does NOT move ================== - `build_field_indexes` and `clear_index_for_point` stay on `StructPayloadIndex`. `build_field_indexes` is read-shaped but only has write-side callers, and pulls in the `selector` machinery which uses `path` + `storage_type`. Keeping it on the writable struct means `path` and `is_appendable` do not need to leak into the view. - The `selector` / `selector_with_type` helpers stay on the writable struct for the same reason. - The free helpers in `query_optimization/condition_converter.rs` (range / geo / null / is-empty checkers) stay where they are; their visibility is bumped from `fn` to `pub(in crate::index)` so the view can still call them. Module layout ============= lib/segment/src/index/struct_payload_index/ mod.rs # owning struct + with_view build.rs # write-side build coordination payload_index.rs # impl PayloadIndex (mutating only) tests.rs read_view/ mod.rs # view struct + module wiring payload_index_read.rs # impl PayloadIndexRead for view filtering.rs # struct_filtered_context, condition_cardinality, query_field, estimate_field_condition condition_converter.rs # impl block from query_optimization/ optimizer.rs # impl block from query_optimization/ value_retriever.rs # impl block from query_optimization/ tests.rs # smoke test that builds the view directly Consumer migration ================== - `Segment::with_view` nests the new `StructPayloadIndex::with_view` inside it; `SegmentReadViewFor<'s>` uses the view as its `TPayloadIndex` parameter. - HNSW (`hnsw.rs`), sparse (`sparse_vector_index.rs`), plain (`plain_vector_index.rs`) call sites wrap their read-method calls in `payload_index.borrow().with_view(|v| ...)`. - `Segment::get_indexed_fields`, `update_all_field_indices`, and `SegmentBuilder::build` switch to `with_view` for `indexed_fields()` / `get_payload_sequential()`. - Integration tests and benches similarly migrate. - `set_payload` (still on `PayloadIndex` write impl) inlines its former `self.get_payload(...)` call as `self.payload.borrow().get(...)` to avoid going through `with_view` from a `&mut self` write path. Smoke test (`read_view/tests.rs`) constructs the view directly over `InMemoryPayloadStorage` + `InMemoryIdTracker` + an empty vector-storage map, and exercises `indexed_fields()`, `query_points()`, and `available_point_count()` -- proving the view is genuinely decoupled from `StructPayloadIndex`. This is the abstraction PR 4 will use to wire a read-only segment. Verified ======== - `cargo build --workspace --tests --benches` -- green - `cargo test -p segment --lib` -- 666 passed (665 + 1 new smoke test), 0 failed - `cargo test -p segment --tests` -- 120 integration tests pass - `cargo test -p storage --lib` -- 44 passed - `cargo test -p collection --lib` -- 197 passed - `cargo clippy -p segment --tests --benches` -- clean Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>