Skip to content

Conversation

@drbh
Copy link

@drbh drbh commented Apr 11, 2023

This is a great project! Here's a small contribution 🙂

Adds insert to Hnsw and HnswMap. The goal is to allow for incremental updates to the index by following these steps.

  1. Add a new point - by finding nearest neighbors at that level and connect them.
  2. Update connections - for each level below the new point's level, find the nearest neighbors at that level and update the connections accordingly (handled by Construction.insert

I added a simple test to validate the insertion and lookup - however I am no expert and would love any feedback to improve this PR! 🙏

Thank you!

@drbh
Copy link
Author

drbh commented Apr 11, 2023

tagging @djc for visibility

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! No need to tag me, I'm pretty good at keeping track of my GitHub notifications -- but also pretty busy so it might take me a bit.

Some(MapItem::from(self.hnsw.get(i, search)?, self))
}

pub fn insert(&mut self, point: P, value: V) -> Result<PointId, Box<dyn std::error::Error>> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to ever result an Err, so it doesn't need to be fallible?

Comment on lines +416 to +420
let zeros = self
.zero
.iter()
.map(|z| RwLock::new(z.clone()))
.collect::<Vec<_>>();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a pretty expensive operation, to the point that calling insert() one-by-one is unlikely to be very effective. Could we yield some construction type here that can be reused to insert multiple points?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants