Skip to content

Propagate getMergeInstance() through the quantized vector readers - #16622

Open
john-mlika wants to merge 1 commit into
apache:mainfrom
john-mlika:sq-reader-merge-instance
Open

john-mlika wants to merge 1 commit into
apache:mainfrom
john-mlika:sq-reader-merge-instance

Conversation

@john-mlika

Copy link
Copy Markdown
Contributor

MergeState asks every reader for a merge instance when a merge starts, and
Lucene99HnswVectorsReader passes that on to its flat reader. For quantized fields the flat reader
is Lucene104ScalarQuantizedVectorsReader (or, for older segments, the Lucene99 and Lucene102
quantized readers in backward-codecs), and that's where the request dies: these readers wrap a
rawVectorsReader but don't override getMergeInstance(), so they return this and the raw reader
never hears about the merge. Same for finishMerge().

What that costs depends on the raw reader. In Lucene it is Lucene99FlatVectorsReader, whose merge
instance is the same reader with its .vec input switched to sequential read advice, so quantized
fields miss an optimization that plain float fields already get on every merge. A raw reader that
keeps a separate input for merging would hand that back instead, and it never gets the chance
either.

I ran into this in Elasticsearch, where the direct-I/O vector stack has one reader for searches and
one for merges and chooses between them in getMergeInstance(). On int8_hnsw fields the choice
never happened, so every merge read the whole source .vec through the search reader. With direct
I/O that is one 8 KiB device round-trip at a time, and twice over the file, once for the integrity
check and once for the copy. Jim Ferenczi fixed the links Elasticsearch owns in
elastic/elasticsearch#153423 and left this one in the commit message: "Lucene104ScalarQuantizedVectorsReader
(int8_hnsw / int4_hnsw) overrides neither method, so nothing below it is reached for those field
types. It keeps its rawVectorsReader private with no copy constructor, so the equivalent fix cannot
be made from the Elasticsearch subclass and belongs upstream." So here it is.

The change is small. Each reader gets a copy constructor that shares the original's open state and
takes a different rawVectorsReader. getMergeInstance() returns a copy built around
rawVectorsReader.getMergeInstance(), and finishMerge() forwards to the raw reader. That is how
Lucene99HnswVectorsReader already handles its own flat reader. The fields map moves out of its
field initializer into the main constructor, because a final field with an initializer can't be
assigned by the copy constructor. On the two non-final readers the copy constructor is
protected, and the getMergeInstance() javadoc explains that a subclass with state of its own has
to override it and build its own copy, or it gets a plain base-class merge instance.
Lucene99ScalarQuantizedVectorsReader is final, so its constructor stays private.

A merge instance is a view over the live reader's resources: the merging thread uses it and nobody
closes it, the same as the HNSW readers' merge instances. If the raw reader's getMergeInstance()
returns this, the copy reads through the same objects and the only effect is whatever the raw
reader did along the way. No file format changes.

I included the two backward-codecs readers because they still serve every merge whose sources are
pre-10.4 scalar-quantized or 10.2 binary-quantized segments, which is what an upgrade merge reads
for anyone who had quantized vectors before 10.4. I can split them into a follow-up if you'd rather
keep this to core.

A few limits. This only matters when the merge reads through a pooled reader, so on an index that is
serving searches, or NRT. Without a pooled reader, IndexWriter opens the sources with a merge
context that MMapDirectory already maps to sequential advice. Merges that go through
MergePolicy#reorder or addIndexes(CodecReader...) are wrapped in readers that don't propagate
merge instances at all, before or after this. And while a merge runs, a quantized field's .vec is
on sequential advice for concurrent searches that read full-precision vectors, which is already the
case for plain float fields.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant