[Rust] Build DiskANN index by eddyxu · Pull Request #763 · lance-format/lance

eddyxu · 2023-04-07T23:16:37Z

No description provided.

changhiskhan · 2023-04-15T20:27:01Z

+}
+
+impl DiskANNParams {
+    pub(crate) fn byte_length(&self) -> usize {


should this be num_sub_vectors * num_bits?

seems "accidentally" correct just because we happen to use 1 byte pq code

This should be depended on how PQ is refactored later, i.e,. if nbits=4, one byte can be two PQ code, it is bit difficult to put the logic like ceiling(nbits / 8) hard coded here.

changhiskhan · 2023-04-15T20:33:12Z

+pub use persisted::*;
+
+/// Graph build parameters.
+pub trait GraphParams {}


what are we going to use this for?

changhiskhan · 2023-04-15T20:36:19Z

        return Err(Error::Index("Invalid graph".to_string()));
    }
-    let binary_size = graph.vertex(0).byte_length();
+    let binary_size = graph.vertex(0).byte_length(graph_params);


Would it make more sense to have Vertex/Node factories that just takes a graph_params?
That way instead of passing it explicitly in a bunch of places, you give the GraphParams to like the GraphBuilder and then forget about it? And the GraphBuilder is responsible for making the nodes/vertices?

Made a new VertexSerDe trait.

changhiskhan · 2023-04-15T20:37:25Z

+        self.nodes[id].neighbors = neighbors.into();
+    }
+
    pub fn add_edge(&mut self, from: usize, to: usize) {


nit pick on naming: why is set_neighbors but add_edge?

changhiskhan · 2023-04-15T20:38:22Z

+        Arc::new(dataset)
+    }
+
+    // #[tokio::test]


why commented out?

changhiskhan · 2023-04-15T20:54:18Z

+        .row(idx)
+        .ok_or(Error::Index("Invalid row index".to_string()))?;
+
+    let dists = l2_distance(query, vector, vectors.num_columns())?;


TODO: support other distance metrics?

changhiskhan · 2023-04-15T20:55:10Z

+    Ok(())
+}
+
+/// Write index file.


so right now nothing is actually writing the index to disk?

Now it only write graphs to disk (as *.lance), the metadata (protobuf) will be written in the following PRs.

changhiskhan · 2023-04-15T20:59:03Z

+        .ok_or_else(|| Error::Index("Cannot find the medoid of an empty matrix".to_string()))?;
+
+    let dist_func = metric_type.func();
+    // Find the closest vertex to the centroid.


i don't think this is the definition of the medoid is it? I thought medoid is the element in a cluster whose distance to all of the other elements is minimized.

They seems to start to use "closest one to centroid" since the FreshDiskANN paper, maybe due to the time complexity of finding medoid.

I can change the function name tho.

changhiskhan · 2023-04-15T21:03:51Z

+    let mut candidates: BTreeMap<OrderedFloat<f32>, usize> = BTreeMap::new();
+    let mut heap: BinaryHeap<VertexWithDistance> = BinaryHeap::new();
+    let dist = distance_to(vectors, query, start)?;
+    heap.push(VertexWithDistance {


the heap is never more than one element?

so why use the heap at all?

Got lost of a few coommits. Refactored and fixed.

changhiskhan · 2023-04-15T21:05:07Z

+        }
+    }
+
+    Ok((


so currently this is searching through all neighbors of the starting vertex and returning the K closest?

changhiskhan

couple of nits

changhiskhan · 2023-04-17T23:59:06Z

+    pub fn func(&self) -> Arc<dyn Fn(&[f32], &[f32]) -> f32> {
+        match self {
+            Self::L2 => Arc::new(l2_distance),
+            Self::Cosine => todo!("cosine distance"),


when does this get used vs batch? does this mean we can't choose cosine distance for index?

i am still refactoring the cosine to linalg module, similar to what does to l2 in #781 , after that, will have a cosine function returned here.

changhiskhan · 2023-04-18T00:05:08Z

+impl VertexSerDe<PQVertex> for PQVertexSerDe {
+    #[inline]
+    fn size(&self) -> usize {
+        8 /* row_id*/ + 8 /* Length of pq */ + self.num_sub_vectors * 1 /* only 8 bits now */


only 8 bits now => isn't it parameterized above as pq_num_bits?

what's "length of pq" (8) ?

only 8 bits now => isn't it parameterized above as pq_num_bits?

we have not finalized how to materialize pq bits yet. i.e., 4bits and 12 bits, whether it takes ceiling or compress a few pq into the shared bytes.

what's "length of pq"

Length of the PQ code in the vector, i.e., sub_vectors.

changhiskhan · 2023-04-18T00:19:39Z

+    fn distance(&self, a: usize, b: usize) -> Result<f32> {
+        let vector_a = self.data.row(a).ok_or_else(|| {
+            Error::Index(format!(
+                "Attempt to access row {} in a matrix with {} rows",


wht does this mean?

Means the vector id is out of bound (of what are hold in the matrix view). Suggestion for rewording?

changhiskhan · 2023-04-18T00:20:18Z

-    pub fn add_edge(&mut self, from: usize, to: usize) {
-        self.nodes[from].neighbors.push(to as u32);
+    /// Set neighbors of a node.
+    pub fn set_neighbors(&mut self, id: usize, neighbors: impl Into<Vec<u32>>) {


should this be vertex as well?

vertex? or Graph?

The graph interface will be shared with persisted graph as well, so the greedy_search will shared between index and graph builder.

eddyxu force-pushed the lei/diskann branch 3 times, most recently from 0158d72 to db61c17 Compare April 15, 2023 17:28

eddyxu marked this pull request as ready for review April 15, 2023 18:46

eddyxu requested a review from changhiskhan April 15, 2023 18:46

changhiskhan reviewed Apr 15, 2023

View reviewed changes

eddyxu force-pushed the lei/diskann branch from 1fe21f2 to 22e3884 Compare April 16, 2023 21:35

eddyxu added 10 commits April 17, 2023 16:15

buildable

cab73be

use col names

4e5b80e

metric

06a1a13

fix

33459b0

make distance to be part of graph trait

1e339dd

fix bunch

1f14e37

move distance to function to graph

136ae89

test

01918ba

row

2d79d52

rebase to main

f01cca1

eddyxu force-pushed the lei/diskann branch from 63fc1bf to f01cca1 Compare April 17, 2023 23:20

only serialize row id

f379086

eddyxu changed the title ~~[Rust] Write diskann index~~ [Rust] Build DiskANN index Apr 17, 2023

changhiskhan approved these changes Apr 18, 2023

View reviewed changes

wording

a5f8a56

eddyxu merged commit 220dc8f into main Apr 18, 2023

eddyxu deleted the lei/diskann branch April 18, 2023 02:59

+                      }
+                  }
+                  Ok((

Conversation

eddyxu commented Apr 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changhiskhan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants