Uh oh!
There was an error while loading. Please reload this page.
[diskann-utils] Simplify StridedView. - #1376
[diskann-utils] Simplify StridedView.#1376Mark Hildebrand (hildebrandmw) wants to merge 4 commits into
StridedView.#1376Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a new unsafe, pointer-based core view type and manual auto-trait impls, which warrants careful human review despite largely mechanical downstream updates.
Pull request overview
This PR refactors diskann-utils’ strided matrix view abstraction by collapsing prior owned/mutable view variants into a single borrowed Strided<'a, T> with an explicit validated Layout, and updates downstream quantization/benchmark/provider code to use the new API.
Changes:
- Replaced
StridedView/MutStridedView/StridedBasewithStrided<'a, T>, plus aLayoutvalidator and a non-panickingRowsiterator. - Updated PQ training, k-means, transposed tables, providers error bridging, and benchmark recall code to use
Strided::try_from_data(...),rows(), and new accessors. - Introduced a small
internalhelper for slice-to-NonNullconversion to support the new representation.
File summaries
| File | Description |
|---|---|
| diskann-utils/src/strided.rs | Replaces the old strided view types with Strided<'a, T>, adds Layout validation, new element/row accessors, and a custom Rows iterator; updates tests accordingly. |
| diskann-utils/src/lib.rs | Adds the new internal module to support Strided’s representation. |
| diskann-utils/src/internal.rs | Adds an internal helper for creating NonNull from a slice base pointer. |
| diskann-quantization/src/product/train.rs | Switches PQ training chunk views from StridedView to Strided and uses rows() instead of row_iter(). |
| diskann-quantization/src/product/tables/transposed/table.rs | Updates pivot slicing to construct Strided via try_from_data. |
| diskann-quantization/src/product/tables/transposed/pivots.rs | Updates APIs and call sites to accept Strided and uses rows() / row_unchecked() access. |
| diskann-quantization/src/multi_vector/block_transposed.rs | Updates block-transpose construction to accept Strided and uses row_unchecked(). |
| diskann-quantization/src/algorithms/kmeans/plusplus.rs | Updates kmeans++ helpers to consume Strided and uses row_or_panic() for copying selected points. |
| diskann-quantization/src/algorithms/kmeans/lloyds.rs | Updates Lloyd’s implementation to consume Strided and iterate with rows(). |
| diskann-providers/src/model/pq/strided.rs | Updates error-bridging to the new non-generic TryFromError type. |
| diskann-benchmark-core/src/recall.rs | Updates optional distance matrix handling to accept Strided and uses row_or_panic(). |
Review details
Suppressed comments (2)
diskann-utils/src/strided.rs:329
- The doc comment says
TryFromErroris forStrided::new, but there is nonewconstructor; the fallible constructor istry_from_data. This makes rustdoc misleading for callers.
/// Errors for [`Strided::new`].
#[derive(Debug, Error)]
diskann-utils/src/strided.rs:353
- The
From<MatrixView>impl can panic via.expect(...).Fromis expected to be infallible; if this conversion is genuinely fallible (overflow or length mismatch), preferTryFrom<MatrixView>(and/or keepFromonly afterMatrixViewinvariants are strengthened).
matrix.nrows(),
matrix.ncols(),
matrix.ncols(),
)
.expect("this will be made infallible in the future")
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #1376 +/- ##
==========================================
- Coverage 91.55% 91.53% -0.02%
==========================================
Files 521 522 +1 Lines 100302 100195 -107 ==========================================
- Hits 91828 91716 -112 - Misses 8474 8479 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
We apparently don't need owned or mutable
StridedViews. This struct was getting in the way of simplifying theMatrixtype, so I decided to do the following:Strided<'a, T>struct.Stridedis well formed.Rowsiterator so we no longer panic if the column-stride is zero.Note that currently,
From<MatrixView<'a, T>>can panic. This is becauseMatrixViewand friends don't actually guarantee thatnrows * ncolsdoes not overflowusize::MAX. I have another PR in the pipeline to fix that, but ran into a chicken and egg problem withStridedpreventing that PR from making progress.