You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "john-mlika (via GitHub)" <gi...@apache.org> on 2026/09/02 16:25:29 UTC
[PR] Propagate getMergeInstance() through the quantized vector readers [lucene]
john-mlika opened a new pull request, #16622:
URL: https://github.com/apache/lucene/pull/16622
`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.
--
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] Propagate getMergeInstance() through the quantized vector readers [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi merged PR #16622:
URL: https://github.com/apache/lucene/pull/16622
--
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] Propagate getMergeInstance() through the quantized vector readers [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on PR #16622:
URL: https://github.com/apache/lucene/pull/16622#issuecomment-5956882012
Thanks for this, and sorry it has been sitting.
A few things moved under it. #16749 landed, so `Lucene99FlatVectorsReader.getMergeInstance()` now reopens the `.vec` through its own mapping, sequential and no-reuse, released by `finishMerge()`. That makes this change more valuable than when you opened it: the gap no longer costs just a read advice hint, it means quantized fields never get the separate mapping at all.
Your approach still looks right to me. It forwards to the raw reader instead of reimplementing anything, so I don't think you need to copy what #16749 did, only to reach it.
Two things that changed meaning though:
`finishMerge()` forwarding is now load-bearing rather than symmetry. The flat reader's merge instance holds a refcounted mapping, so without the forward it stays open until the reader is closed.
`Lucene104ScalarQuantizedVectorsReader` has changed under #16682, #16687, #16696 and #16705, so the PR conflicts today and the copy constructors may need adjusting.
Could you rebase on main and confirm the shape still fits? Happy to review after that.
--
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] Propagate getMergeInstance() through the quantized vector readers [lucene]
Posted by "john-mlika (via GitHub)" <gi...@apache.org>.
john-mlika commented on PR #16622:
URL: https://github.com/apache/lucene/pull/16622#issuecomment-6004037253
i rebased on main and the copy constructors didn't need changes. finishMerge is forwarded and the tests make sure that a merge closes what it opened. i also reworked the tests a bit since the old ones were asserting that the read advice is switched which has been removed in #16749. please have a look @jimczi
--
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