You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "benwtrent (via GitHub)" <gi...@apache.org> on 2023/10/27 19:42:57 UTC
[PR] Adding new flat vector format and refactoring HNSW [lucene]
benwtrent opened a new pull request, #12729:
URL: https://github.com/apache/lucene/pull/12729
Currently the HNSW codec does too many things, it not only indexes vectors, but stores them and determines how to store them given the vector type.
This PR extracts out the vector storage into a new format `Lucene99FlatVectorsFormat`, additionally a "flat" version of `KnnVectorsFormat` is added called `FlatVectorsFormat`. This allows for some additional helper functions that allow an indexing codec (like HNSW) take advantage of the flat formats.
Additionally, this PR refactors the new `Lucene99ScalarQuantizedVectorsFormat` to be a `FlatVectorsFormat`.
Now, `Lucene99HnswVectorsFormat` can accept either a `Lucene99ScalarQuantizedVectorsFormat` to use int8 quantized vectors or `Lucene99FlatVectorsFormat` for raw vectors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #12729:
URL: https://github.com/apache/lucene/pull/12729#discussion_r1380023598
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsReader.java:
##########
@@ -399,41 +281,30 @@ private HnswGraph getGraph(FieldEntry entry) throws IOException {
@Override
public void close() throws IOException {
- IOUtils.close(vectorData, vectorIndex, quantizedVectorData);
+ IOUtils.close(flatVectorsReader, vectorIndex);
}
@Override
- public OffHeapQuantizedByteVectorValues getQuantizedVectorValues(String field)
- throws IOException {
- FieldEntry fieldEntry = fields.get(field);
- if (fieldEntry == null || fieldEntry.hasQuantizedVectors() == false) {
- return null;
+ public QuantizedByteVectorValues getQuantizedVectorValues(String field) throws IOException {
+ if (flatVectorsReader instanceof QuantizedVectorsReader) {
+ return ((QuantizedVectorsReader) flatVectorsReader).getQuantizedVectorValues(field);
Review Comment:
OK, fair enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #12729:
URL: https://github.com/apache/lucene/pull/12729#discussion_r1376303898
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsReader.java:
##########
@@ -399,41 +281,30 @@ private HnswGraph getGraph(FieldEntry entry) throws IOException {
@Override
public void close() throws IOException {
- IOUtils.close(vectorData, vectorIndex, quantizedVectorData);
+ IOUtils.close(flatVectorsReader, vectorIndex);
}
@Override
- public OffHeapQuantizedByteVectorValues getQuantizedVectorValues(String field)
- throws IOException {
- FieldEntry fieldEntry = fields.get(field);
- if (fieldEntry == null || fieldEntry.hasQuantizedVectors() == false) {
- return null;
+ public QuantizedByteVectorValues getQuantizedVectorValues(String field) throws IOException {
+ if (flatVectorsReader instanceof QuantizedVectorsReader) {
+ return ((QuantizedVectorsReader) flatVectorsReader).getQuantizedVectorValues(field);
Review Comment:
Should we have that method on the flat vectors reader?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1785563758
OK, @jpountz thinking about it more. To do what you are suggesting, I think the following would work:
- Force Lucene99HnswVectorsReader & Lucene99HnswVectorsWriter to take a `FlatVectors[Read|Writer]`. This way the hard coded `*Format` files can reuse all the common code.
- Remove the SPI the formats are now required to indicate how the vectors are stored and read
- Remove the inheritance tree for the `FlatVector*` things so that they are not `knn` things.
In a separate PR I guess we can talk about a "flat" vector codec and if that is desired or not.
The "*Flat" things now get users very far, they just need to have a custom codec that provides a couple lines of code to forward the searching, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1792590687
> The flat format is an implementation detail. Folks using the quantized hnsw do not have to supply a flat format.
We can register the flat format for direct usage (outside of HNSW) and use them in the HNSW formats as an implementation detail? My point is that we don't need the `FlatVector...` classes. We can expose the quantized hnsw format with the quantiles option and use the scalar flat format internally. Users won't have to supply the flat format explicitly but it will be used internally. For instance if we introduce the half-float format at a later stage as `Float16FlatVectorsFormat`, we would need to expose a `HNSWFloat16VectorsFormat` to make it available with HNSW. In other words, we don't allow composable formats explicitly but each high level format can reuse other format internally (as an implementation detail).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1792686970
@jimczi the HNSWWriter and Readers need the passed flat vector readers and writers to provide specific functions. Like the mergeOneField that returns closeable scorers.
I am not sure all knnVectorReaders/Writers should have that method.
I think we should expose the flat formats in the codec. But the required new functions for indexing the vectors seem to justify a new abstraction.
Or do you think they are just KnnReader/Writers and we update that base abstraction?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1804243772
Sorry for the late reply.
> Since this is a larger API discussion, do we think we can move forward with the way it is now (quantization for HNSW and other vector indices) and iterate on exposing flat vectors in a separate line of work?
Sure that's fine with me, can you update the title of this PR then to reflect what it's really doing? If we're not adding the flat format in the PR, it's like a cleanup to make code more reusable that don't affect the format, right?
Regarding the API discussion, I think that exposing the RandomValueVectorScorer should be enough for what we need but I am surely missing something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #12729:
URL: https://github.com/apache/lucene/pull/12729#discussion_r1379919203
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsReader.java:
##########
@@ -399,41 +281,30 @@ private HnswGraph getGraph(FieldEntry entry) throws IOException {
@Override
public void close() throws IOException {
- IOUtils.close(vectorData, vectorIndex, quantizedVectorData);
+ IOUtils.close(flatVectorsReader, vectorIndex);
}
@Override
- public OffHeapQuantizedByteVectorValues getQuantizedVectorValues(String field)
- throws IOException {
- FieldEntry fieldEntry = fields.get(field);
- if (fieldEntry == null || fieldEntry.hasQuantizedVectors() == false) {
- return null;
+ public QuantizedByteVectorValues getQuantizedVectorValues(String field) throws IOException {
+ if (flatVectorsReader instanceof QuantizedVectorsReader) {
+ return ((QuantizedVectorsReader) flatVectorsReader).getQuantizedVectorValues(field);
Review Comment:
@jpountz I honestly don't know. It is sort of like having `getHnswGraph` part of the `KnnVectorsReader`. It assumes that this is a thing that all readers will need. The only thing that needs this `getQuantizedVectorValues` is something that is merging `int8` quantized values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1795961973
@jimczi thinking about it more, it seems to be a Flat* index format for vector search will require a different API. Right now, kNN search assumes the user provides pre-filters. However, when doing a flat vector search, there is no such thing as a "pre-filter".
In fact, a flat vector search should behave like a typical scorer, iterating over matching documents and scoring them.
The question then becomes, could we just use the `LeafReader#getFloatVectorValues`?
We might be able to, but this could be a bigger change and might not buy a big improvement. It seems counter intuitive to de-quantize every vector back into a `float[]` instead of just quantizing the query vector into `byte[]`.
It almost seems like these flat formats should return a `RandomVectorScorer` instead of satisfying the typical `searchNearestVectors` interface.
Since this is a larger API discussion, do we think we can move forward with the way it is now (quantization for HNSW and other vector indices) and iterate on exposing flat vectors in a separate line of work?
@jimczi @jpountz if y'all have ideas around how the API could look, please let me know!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1786059581
@jpountz updated. Flat is no longer pluggable, two HNSW formats are exposed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #12729:
URL: https://github.com/apache/lucene/pull/12729#discussion_r1379282330
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsReader.java:
##########
@@ -399,41 +281,30 @@ private HnswGraph getGraph(FieldEntry entry) throws IOException {
@Override
public void close() throws IOException {
- IOUtils.close(vectorData, vectorIndex, quantizedVectorData);
+ IOUtils.close(flatVectorsReader, vectorIndex);
}
@Override
- public OffHeapQuantizedByteVectorValues getQuantizedVectorValues(String field)
- throws IOException {
- FieldEntry fieldEntry = fields.get(field);
- if (fieldEntry == null || fieldEntry.hasQuantizedVectors() == false) {
- return null;
+ public QuantizedByteVectorValues getQuantizedVectorValues(String field) throws IOException {
+ if (flatVectorsReader instanceof QuantizedVectorsReader) {
+ return ((QuantizedVectorsReader) flatVectorsReader).getQuantizedVectorValues(field);
Review Comment:
Bump: this sounds like a good addition to the flat vectors reader?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactoring HNSW to use a new internal FlatVectorFormat [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1804353255
@jimczi updated the title.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1792391067
I agree with Adrien that hardcoded formats with a clear strategy are better.
We want to avoid exposing a knn format that takes another abstract format. That would be cryptic and difficult to navigate for users.
However I think we can reuse existing format as an implementation detail. The `Lucene99HnswScalarQuantizedVectorsFormat ` could be hardcoded to use the `Lucene99ScalarQuantizedVectorsFormat` internally. The fact that we register the format does not mean that we expose the composability to users. In its current form the `FlatVector...` lineage is redundant if it's only to avoid to register the format explicitly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactoring HNSW to use a new internal FlatVectorFormat [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent merged PR #12729:
URL: https://github.com/apache/lucene/pull/12729
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1785376897
@jpountz the goal of this change is not just making code reusable. But:
- Allowing folks who don't want HNSW to take advantage of the per-segment quantization and logic. Paging in float32 values, even when not using HNSW, can take time and memory. Reducing the size and just using `byte` values is really attractive.
- being able to plugin a different flat storage no matter the vector indexer is useful as I would think we will want to add other quantizations in the future (e.g. `bfloat16`).
I suppose your suggestion around separate HNSW formats with flat storage hardcoded can still satisfy my second point, as long as the classes are usable.
But, I would really like to not lose the first goal, making quantization available to users who want flat vector storage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1792546852
@jimczi what do you mean "existing format as implementation detail"?
The flat format is an implementation detail. Folks using the quantized hnsw do not have to supply a flat format.
And by "register format" do you mean we should register the flat formats? Right now they are just classes that encapsulate vector storage and merging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1792854167
> I think we should expose the flat formats in the codec. But the required new functions for indexing the vectors seem to justify a new abstraction.
Can we add the abstraction as an extension of the existing knnVectorFormat/Readers/Writers? So instead of adding the functions to the base class, we introduce the flat format as an abstract class that extends the base one.
That would allow to reference it on the HNSW format directly and all flat formats would inherit from it directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Adding new flat vector format and refactoring HNSW [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #12729:
URL: https://github.com/apache/lucene/pull/12729#issuecomment-1785712360
Thanks, splitting the way you describe would make me happy.
I had not understood that the flat codec was a goal. Now that I think more about it, I wonder if we should better separate storage and indexing of vectors in FieldType/FieldInfo and file formats. E.g. it's a bit awkward that a `KnnVectorQuery` would try to reason about whether it should use the index or flat storage when in fact it only has flat storage. Maybe it could look more like keywords, where you decide separately whether to index them (index options) and/or store them (doc values).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org