-
Notifications
You must be signed in to change notification settings - Fork 139
New MutableGraphIndex and ImmutableGraphIndex interfaces #534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ide implementations
Before you submit for review:
If you did not complete any of these, then please explain below. |
@sam-herman @michaeljmarshall Please make sure that the new design does not preclude any usages of JVector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should work well with #519
jvector-base/src/main/java/io/github/jbellis/jvector/graph/GraphIndexBuilder.java
Show resolved
Hide resolved
jvector-base/src/main/java/io/github/jbellis/jvector/graph/ImmutableGraphIndex.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks good. I pulled the PR branch to my machine, built it, imported it into CC main
(datastax/cassandra@748018e), fixed compilation errors, and ran VectorUpdateDeleteTest
and didn't have any test failures.
I left some minor comments mostly on naming/javadocs and one comment about the getView()
method that will impact CC.
jvector-base/src/main/java/io/github/jbellis/jvector/graph/MutableGraphIndex.java
Outdated
Show resolved
Hide resolved
jvector-base/src/main/java/io/github/jbellis/jvector/graph/MutableGraphIndex.java
Show resolved
Hide resolved
jvector-base/src/main/java/io/github/jbellis/jvector/graph/OnHeapGraphIndex.java
Outdated
Show resolved
Hide resolved
jvector-base/src/main/java/io/github/jbellis/jvector/graph/MutableGraphIndex.java
Outdated
Show resolved
Hide resolved
jvector-base/src/main/java/io/github/jbellis/jvector/graph/MutableGraphIndex.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…is being mutated by GraphIndexBuilder and a FrozenView once mutations are done.
jvector-base/src/main/java/io/github/jbellis/jvector/graph/OnHeapGraphIndex.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix.
The main intent of this PR is to add an interface in front of the OnHeapGraphIndex. This is because it has become clear recently that we will need new implementations for mutable graphs (that are more conservative in the use of heap memory). This PR paves the way towards that path.
Importantly, MutableGraphIndex is protected inside its package, because modifications should only be done through the GraphIndexBuilder class and never directly.
These changes triggered a lot of minor changes everywhere.