LUCENE-9583: Remove RandomAccessVectorValuesProducer #1071
Conversation
There was a problem hiding this comment.
@jtibshirani Thanks! Very nice refactoring and simplificaiton!
| @@ -192,36 +176,5 @@ public int advance(int target) throws IOException { | |||
| public long cost() { | |||
| return size(); | |||
| } | |||
|
|
|||
| @Override | |||
| public RandomAccessVectorValues randomAccess() throws IOException { | |||
There was a problem hiding this comment.
Very nice simplification!
There was a problem hiding this comment.
This is kind of funny to me since it is kind of back to a much earlier iteration where these were all on the same interface, but I split them apart as a way to try and hide the random access that people didn't like :) Although it was never very successful. Anyway I am +1 to get rid of the fig leaf
There was a problem hiding this comment.
Indeed we never reached a great resolution there! I'm trying to chip away at some of these tricky design questions, we'll see how it goes...
lucene/core/src/java/org/apache/lucene/util/hnsw/HnswGraphBuilder.java
Outdated
Show resolved
Hide resolved
| @@ -308,41 +302,6 @@ private void printFanoutHist(Path indexPath) throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| @SuppressWarnings("unchecked") | |||
| private void dumpGraph(Path docsPath) throws IOException { | |||
There was a problem hiding this comment.
hm I guess we can always restore. This had been pretty useful in early times when nothing was working ...
There was a problem hiding this comment.
Let me know anytime you start to miss it and I'll restore it! For now I'll remove it to keep things simple. If we keep it, I'll need to refactor BinaryFileVectors -- which is doable but slightly tricky.
| @@ -192,36 +176,5 @@ public int advance(int target) throws IOException { | |||
| public long cost() { | |||
| return size(); | |||
| } | |||
|
|
|||
| @Override | |||
| public RandomAccessVectorValues randomAccess() throws IOException { | |||
There was a problem hiding this comment.
This is kind of funny to me since it is kind of back to a much earlier iteration where these were all on the same interface, but I split them apart as a way to try and hide the random access that people didn't like :) Although it was never very successful. Anyway I am +1 to get rid of the fig leaf
|
Thank you for the reviews. I'm going to merge since it seems you're both on board with the change. |
This change folds the `RandomAccessVectorValuesProducer` interface into `RandomAccessVectorValues`. This reduces the number of interfaces and clarifies the cloning/ copying behavior. This is a small simplification related to LUCENE-9583, but does not address the main issue.
This change folds the
RandomAccessVectorValuesProducerinterface intoRandomAccessVectorValues. This reduces the number of interfaces and clarifiesthe cloning/ copying behavior.
This is a small simplification related to LUCENE-9583, but does not address the
main issue.