You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "LantaoJin (via GitHub)" <gi...@apache.org> on 2026/06/08 06:27:55 UTC
[PR] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
LantaoJin opened a new pull request, #16214:
URL: https://github.com/apache/lucene/pull/16214
### Description
Adds two `IndexWriter` APIs to update a document's KNN vector **in place**, without reindexing the
rest of the document:
```java
public long updateFloatVectorValue(Term term, String field, float[] value)
public long updateByteVectorValue(Term term, String field, byte[] value)
```
Today the only way to change a stored embedding (`KnnFloatVectorField` / `KnnByteVectorField`) is
`updateDocument` -- delete-by-term + re-add the **whole** document, which re-analyzes/re-posts/
re-stores every field.
For workloads that periodically re-embed (e.g. bumping the embedding model
version) that is wasteful: only the vector changed. These APIs mirror `updateDocValues` -- they
rewrite just the affected field at a new per-segment generation and leave everything else untouched.
### Benchmark summary
`dim=768, otherFields=8`, ms per commit (lower is better):
| numDocs | batchSize | updateVectorValue | updateDocument |
|--:|--:|--:|--:|
| 50000 | 1 | 238 | 9 |
| 50000 | 1000 | 251 | 542 |
| 50000 | 10000 | 384 | 8090 |
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on PR #16214:
URL: https://github.com/apache/lucene/pull/16214#issuecomment-4678996735
Thanks @jimczi , closed
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on PR #16214:
URL: https://github.com/apache/lucene/pull/16214#issuecomment-4678048199
> the same deferral updateDocument / updateDocValues apply to graph / secondary-structure work
I don’t think this is quite how it works. `updateDocValues` is only possible when the field is indexed purely as doc values. That’s really the trade-off here: we probably don’t want different update semantics depending on the underlying data structure.
> re-embed the whole vector field after a model fine-tune (the motivating case).
I also think this use case is a bit misleading. If the model changes, you generally need to update all vectors anyway, otherwise you end up mixing embeddings from different models and queries become inconsistent/wrong. In that case, reindexing into a new index in the background and switching over atomically once it’s done feels like the cleaner approach.
As it stands, I don’t think this change is in an acceptable state yet. The semantics feel too tied to specific implementations/data structures, and the motivating use case doesn’t really justify the complexity or trade-offs being introduced.
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on code in PR #16214:
URL: https://github.com/apache/lucene/pull/16214#discussion_r3377644647
##########
lucene/core/src/java/org/apache/lucene/index/BufferedUpdates.java:
##########
@@ -155,6 +159,15 @@ void addBinaryUpdate(BinaryDocValuesUpdate update, int docIDUpto) {
numFieldUpdates.incrementAndGet();
}
+ void addVectorUpdate(KnnVectorUpdate update, int docIDUpto) {
+ FieldUpdatesBuffer buffer =
+ vectorUpdates.computeIfAbsent(
Review Comment:
The `FieldUpdatesBuffer ctor` already stores the initial update, so I now only call `addUpdate()` for subsequent updates to the same field.
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on code in PR #16214:
URL: https://github.com/apache/lucene/pull/16214#discussion_r3377644647
##########
lucene/core/src/java/org/apache/lucene/index/BufferedUpdates.java:
##########
@@ -155,6 +159,15 @@ void addBinaryUpdate(BinaryDocValuesUpdate update, int docIDUpto) {
numFieldUpdates.incrementAndGet();
}
+ void addVectorUpdate(KnnVectorUpdate update, int docIDUpto) {
+ FieldUpdatesBuffer buffer =
+ vectorUpdates.computeIfAbsent(
Review Comment:
The `FieldUpdatesBuffer` ctor already stores the initial update, so I now only call `addUpdate()` for subsequent updates to the same field.
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on code in PR #16214:
URL: https://github.com/apache/lucene/pull/16214#discussion_r3377642047
##########
lucene/core/src/java/org/apache/lucene/index/KnnVectorFieldUpdates.java:
##########
@@ -0,0 +1,335 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+import static org.apache.lucene.search.DocIdSetIterator.NO_MORE_DOCS;
+
+import java.util.Comparator;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.util.ArrayUtil;
+import org.apache.lucene.util.PriorityQueue;
+
+/**
+ * Holds in-place KNN vector updates of a single field, for a set of documents within one segment.
+ * This is the vector analogue of {@link DocValuesFieldUpdates}: it accumulates (docID -> new
+ * vector) entries during resolution, sorts them by docID at {@link #finish()} (stable, so that the
+ * last update to a docID within a packet wins), and exposes an {@link Iterator} that the write path
+ * overlays onto the existing vectors.
+ *
+ * @lucene.experimental
+ */
+abstract class KnnVectorFieldUpdates {
+
+ final String field;
+ final VectorEncoding encoding;
+ final long delGen;
+ final int maxDoc;
+ final int dimension;
+ protected int[] docs = new int[8];
+ protected int size;
+ private boolean finished;
+
+ protected KnnVectorFieldUpdates(
+ int maxDoc, long delGen, String field, VectorEncoding encoding, int dimension) {
+ this.maxDoc = maxDoc;
+ this.delGen = delGen;
+ this.field = field;
+ this.encoding = encoding;
+ this.dimension = dimension;
+ }
+
+ /** Reserves a slot for the next doc and returns its ordinal. Subclasses store the value. */
+ protected final int reserve(int doc) {
+ if (finished) {
+ throw new IllegalStateException("already finished");
+ }
+ assert doc < maxDoc;
+ if (size == docs.length) {
+ docs = ArrayUtil.grow(docs, size + 1);
+ }
+ docs[size] = doc;
+ return size++;
+ }
+
+ boolean getFinished() {
+ return finished;
+ }
+
+ boolean any() {
+ return size > 0;
+ }
+
+ /** Freezes structures and stably sorts updates by docID (last write to a docID wins). */
+ final void finish() {
Review Comment:
Agreed. Replaced with the in-place `IntroSorter` + `int[] ords` pattern from `DocValuesFieldUpdates.finish()` -- stable tie-break preserved, zero boxing, no extra arrays.
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on PR #16214:
URL: https://github.com/apache/lucene/pull/16214#issuecomment-4666315741
> in that situation you typically need to reindex everything
Thanks @jimczi, @iverase — this is the right thing to pressure-test. Let me give the concrete shape of the use case, because I think it narrows the disagreement.
**The index**: one vector field + ~49 other fields (analyzed text, stored, doc-values). The 49 fields are **immutable** for the lifetime of the index. Only the embedding changes: we fine-tune **model1 → model2** (same dimension, same similarity), and want to refresh the vector field for the affected docs.
You're right that the **vector field itself is rebuilt** regardless -- every re-embedded doc changes its vector, so the flat column is fully rewritten and the HNSW graph fully rebuilt no matter which path we take. In-place doesn't claim to save any vector-side work, and I agree that "you reindex the vector field anyway" is true.
The cost it removes is the **other 49 fields**. Today the only way to refresh the vectors is to build a new index with all 50 fields and drop the old one -- which re-analyzes and rewrites the 49 unchanged fields for zero benefit, and requires the caller to still **possess every field's source value** (often it lives in a separate source-of-truth system and isn't cheaply reconstructable). `updateDocument` has the same problem: it's whole-document, so it drags the 49 fields along. In our indices the 49 fields are the bulk of the indexing cost, so skipping them is the actual win -- not the vector write.
So I'd reframe the value prop as: update the embedding given only `(id, newVector)`, without rebuilding or even possessing the rest of the document. The model-version bump is one instance; partial/rolling re-embedding of a subset is another.
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase commented on PR #16214:
URL: https://github.com/apache/lucene/pull/16214#issuecomment-4660885756
Changing approximated search to exact search under the hood just because of an update feels wrong.
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on PR #16214:
URL: https://github.com/apache/lucene/pull/16214#issuecomment-4667122608
### Usage
The API resolves `term` to the matching document(s) — exactly like `updateDocValues` — and overlays
the new vector on `field`. Address each document by a **unique id term** (an un-analyzed
`StringField`), the same convention as `updateDocument(Term, ...)`.
**Scenario 1 — update a single document's embedding.**
```java
try (IndexWriter writer = new IndexWriter(dir, new IndexWriterConfig())) {
float[] newEmbedding = model.embed(textForDoc42); // 1 x dim, same dim/similarity as the field
writer.updateFloatVectorValue(new Term("id", "doc-42"), "vec", newEmbedding);
writer.commit();
}
```
**Scenario 2 — partial re-embed of a subset (drift correction, fixing a bad batch).**
Only the listed docs are touched; every other document — and all of its fields — is left byte-for-byte
unchanged.
```java
try (IndexWriter writer = new IndexWriter(dir, new IndexWriterConfig())) {
for (Map.Entry<String, float[]> e : reembeddedById.entrySet()) {
writer.updateFloatVectorValue(new Term("id", e.getKey()), "vec", e.getValue());
}
writer.commit(); // each touched segment writes ONE new-generation flat column (not one per call)
}
```
**Scenario 3 — re-embed the whole vector field after a model fine-tune (the motivating case).**
An index with one vector field + N immutable non-vector fields; `model1 → model2` is a fine-tune, so
the dimension and similarity are unchanged. We refresh the embedding for every document **without
rebuilding or possessing the N other fields**, then merge once to rebuild the HNSW graph.
```java
try (IndexReader reader = DirectoryReader.open(dir);
IndexWriter writer = new IndexWriter(dir, new IndexWriterConfig())) {
StoredFields stored = reader.storedFields();
Bits live = MultiBits.getLiveDocs(reader);
for (int docID = 0; docID < reader.maxDoc(); docID++) {
if (live != null && !live.get(docID)) {
continue; // skip deleted docs
}
String id = stored.document(docID).get("id");
float[] newEmbedding = model2.embed(/* source for this doc */);
writer.updateFloatVectorValue(new Term("id", id), "vec", newEmbedding);
}
writer.commit(); // flush the buffered overlays: one new-gen flat column per segment
writer.forceMerge(1); // rebuild the HNSW graph ONCE; merged segment is back to vectorGen == -1
}
```
> Between `commit()` and the merge, the updated segments carry the new flat vectors but no graph, so
> ANN search on them falls back to an exact scan (correct, but slower) — the same deferral
> `updateDocument` / `updateDocValues` apply to graph / secondary-structure work. The `forceMerge`
> rebuilds the graph once, optimally, restoring approximate search. (For workloads that cannot
> tolerate that window, eager rebuild is a planned follow-up — see *Scope / limitations*.)
**Scenario 4 — byte vectors.** Identical shape with `updateByteVectorValue`:
```java
writer.updateByteVectorValue(new Term("id", "doc-42"), "vec", newByteEmbedding); // byte[] of length dim
```
**Scenario 5 — read the update back (committed or NRT).**
```java
writer.updateFloatVectorValue(new Term("id", "doc-1"), "vec", newEmbedding);
try (DirectoryReader reader = DirectoryReader.open(writer)) { // near-real-time, no commit needed
for (LeafReaderContext ctx : reader.leaves()) {
FloatVectorValues values = ctx.reader().getFloatVectorValues("vec");
// ... iterate; doc-1 now reflects newEmbedding, every other doc is unchanged
}
}
```
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on code in PR #16214:
URL: https://github.com/apache/lucene/pull/16214#discussion_r3377663913
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene94/Lucene94FieldInfosFormat.java:
##########
@@ -454,6 +458,8 @@ public void write(
output.writeVInt(fi.getVectorDimension());
output.writeByte((byte) fi.getVectorEncoding().ordinal());
output.writeByte(distFuncToOrd(fi.getVectorSimilarityFunction()));
+ // per-field KNN vector update generation (FORMAT_VECTOR_GEN+)
+ output.writeLong(fi.getVectorGen());
Review Comment:
Yeah, agree! I am thinking the refactor.
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on PR #16214:
URL: https://github.com/apache/lucene/pull/16214#issuecomment-4660728803
I wonder if this is the right tradeoff for ANN-backed indices in general. Losing the ANN structure for the whole segment after a small vector update and falling back to exact search until merge feels fairly expensive and potentially surprising.
Maybe this only really makes sense for flat indices where there is no ANN structure to invalidate?
The use case that feels more important to me is actually the opposite one: updating other fields/doc values without having to rewrite vectors or rebuild the ANN structure.
I saw the “change of model” use case mentioned in the PR, but in that situation you typically need to reindex everything anyway, so I’m not sure this optimization helps much there.
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on code in PR #16214:
URL: https://github.com/apache/lucene/pull/16214#discussion_r3377635868
##########
lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java:
##########
@@ -460,6 +510,187 @@ public long cost() {
}
}
+ /**
+ * Writes a new generation of KNN vector files for each field that has pending vector updates. For
+ * each such field we bump a new {@code vectorGen}, write {@code .vec/.vemf/.vex/.vem} files at
+ * that gen suffix where the vectors are the existing field vectors overlaid with the updates, and
+ * the HNSW graph is rebuilt eagerly (via the standard {@link KnnVectorsWriter} flush path). The
+ * base reader (gen == -1) plus other untouched fields are left in place. Vector analogue of
+ * {@link #handleDVUpdates}.
+ */
+ @SuppressWarnings("unchecked")
+ private synchronized void handleVectorUpdates(
+ FieldInfos infos,
+ Directory dir,
+ KnnVectorsFormat vectorsFormat,
+ final SegmentReader reader,
+ Map<Integer, Set<String>> fieldFiles,
+ long maxDelGen,
+ InfoStream infoStream,
+ boolean deferVectorGraphRebuild)
+ throws IOException {
+ for (Entry<String, List<KnnVectorFieldUpdates>> ent : pendingVectorUpdates.entrySet()) {
+ final String field = ent.getKey();
+ final List<KnnVectorFieldUpdates> updates = ent.getValue();
+ final List<KnnVectorFieldUpdates> updatesToApply = new ArrayList<>();
+ for (KnnVectorFieldUpdates update : updates) {
+ if (update.delGen <= maxDelGen) {
+ updatesToApply.add(update);
+ }
+ }
+ if (updatesToApply.isEmpty()) {
+ continue;
+ }
+
+ // Reject quantized formats: only the unquantized Lucene99 HNSW format is supported for
+ // in-place vector updates.
+ KnnVectorsFormat perFieldFormat = vectorsFormat;
+ if (vectorsFormat instanceof PerFieldKnnVectorsFormat perField) {
+ perFieldFormat = perField.getKnnVectorsFormatForField(field);
+ }
+ if (perFieldFormat instanceof Lucene99HnswVectorsFormat == false) {
+ throw new UnsupportedOperationException(
+ "in-place vector update is only supported for "
+ + Lucene99HnswVectorsFormat.class.getSimpleName()
+ + " but field ["
+ + field
+ + "] uses "
+ + perFieldFormat.getClass().getSimpleName());
+ }
+
+ final long nextVectorGen = info.getNextVectorGen();
+ final String segmentSuffix = Long.toString(nextVectorGen, Character.MAX_RADIX);
+ final IOContext updatesContext = IOContext.flush(new FlushInfo(info.info.maxDoc(), 0));
+
+ // Stamp the new vectorGen onto the (shared, cloned) FieldInfo so it is persisted by
+ // writeFieldInfosGen and picked up at read time. A single-field FieldInfos is handed to the
+ // vectors writer for this gen.
+ final FieldInfo fieldInfo = infos.fieldInfo(field);
+ assert fieldInfo != null && fieldInfo.hasVectorValues();
+ fieldInfo.setVectorGen(nextVectorGen);
+ final FieldInfos fieldInfos = new FieldInfos(new FieldInfo[] {fieldInfo});
+
+ // Choose the writer format for this generation. When deferring the graph rebuild we write
+ // only
+ // the flat vectors and skip building the HNSW graph: the gen's ".vex/.vem" carry an empty
+ // graph
+ // (vectorIndexLength == 0), so the reader falls back to an exact scan on this segment until
+ // the
+ // next merge rebuilds the graph using the codec's normal format. The graph build is
+ // suppressed
+ // by raising the "tiny segment" threshold above the segment size (so shouldCreateGraph() is
+ // always false for this write). We wrap it in a PerFieldKnnVectorsFormat so file naming and
+ // the
+ // per-field suffix attributes match what the (always per-field) reader reconstructs.
+ final KnnVectorsFormat writeFormat;
+ if (deferVectorGraphRebuild) {
+ final KnnVectorsFormat noGraphFormat =
+ new Lucene99HnswVectorsFormat(
+ Lucene99HnswVectorsFormat.DEFAULT_MAX_CONN,
+ Lucene99HnswVectorsFormat.DEFAULT_BEAM_WIDTH,
+ info.info.maxDoc() + 1);
+ writeFormat =
+ new PerFieldKnnVectorsFormat() {
+ @Override
+ public KnnVectorsFormat getKnnVectorsFormatForField(String f) {
+ return noGraphFormat;
+ }
+ };
+ } else {
+ writeFormat = vectorsFormat;
+ }
+
+ final TrackingDirectoryWrapper trackingDir = new TrackingDirectoryWrapper(dir);
+ final SegmentWriteState state =
+ new SegmentWriteState(
+ null, trackingDir, info.info, fieldInfos, null, updatesContext, segmentSuffix);
+
+ try (KnnVectorsWriter writer = writeFormat.fieldsWriter(state)) {
+ final KnnVectorFieldUpdates.Iterator mergedIterator =
+ KnnVectorFieldUpdates.mergedIterator(toIteratorArray(updatesToApply));
+ if (fieldInfo.getVectorEncoding() == VectorEncoding.FLOAT32) {
+ KnnFieldVectorsWriter<float[]> fieldWriter =
+ (KnnFieldVectorsWriter<float[]>) writer.addField(fieldInfo);
+ writeOverlayFloatVectors(reader, field, mergedIterator, fieldWriter);
+ } else {
+ KnnFieldVectorsWriter<byte[]> fieldWriter =
+ (KnnFieldVectorsWriter<byte[]>) writer.addField(fieldInfo);
+ writeOverlayByteVectors(reader, field, mergedIterator, fieldWriter);
+ }
+ writer.flush(info.info.maxDoc(), null);
+ writer.finish();
+ }
+
+ info.advanceVectorGen();
+ assert !fieldFiles.containsKey(fieldInfo.number);
+ fieldFiles.put(fieldInfo.number, trackingDir.getCreatedFiles());
+ }
+ }
+
+ private static KnnVectorFieldUpdates.Iterator[] toIteratorArray(
+ List<KnnVectorFieldUpdates> updatesToApply) {
+ KnnVectorFieldUpdates.Iterator[] subs =
+ new KnnVectorFieldUpdates.Iterator[updatesToApply.size()];
+ for (int i = 0; i < subs.length; i++) {
+ subs[i] = updatesToApply.get(i).iterator();
+ }
+ return subs;
+ }
+
+ /**
+ * Feeds the field writer, in docID order, every doc that currently has a float vector for {@code
+ * field}, substituting the updated vector when the merged update iterator has one for that doc.
+ */
+ private static void writeOverlayFloatVectors(
Review Comment:
Good catch. The overlay now throws an `IllegalArgumentException` instead of skipping an unmatched update.
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on code in PR #16214:
URL: https://github.com/apache/lucene/pull/16214#discussion_r3377657727
##########
lucene/core/src/java/org/apache/lucene/index/SegmentVectorsProducer.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.Set;
+import org.apache.lucene.codecs.KnnVectorsReader;
+import org.apache.lucene.internal.hppc.IntObjectHashMap;
+import org.apache.lucene.internal.hppc.LongArrayList;
+import org.apache.lucene.search.AcceptDocs;
+import org.apache.lucene.search.KnnCollector;
+import org.apache.lucene.store.Directory;
+
+/**
+ * Encapsulates multiple per-generation {@link KnnVectorsReader}s as one reader when there are KNN
+ * vector updates. This is the vector analogue of {@link SegmentDocValuesProducer}.
+ *
+ * <p>Fields with no vector updates (vectorGen == -1) are served from the shared, immutable base
+ * reader held by {@link SegmentCoreReaders}; that base reader is never closed by this producer (its
+ * lifecycle is owned by the core). Fields with updates are served from per-gen readers tracked by
+ * {@link SegmentKnnVectors} via reference counting.
+ */
+class SegmentVectorsProducer extends KnnVectorsReader {
+
+ private final FieldInfos fieldInfos;
+ final IntObjectHashMap<KnnVectorsReader> readersByField = new IntObjectHashMap<>();
+ // distinct gen readers (excluding the base core reader) for checkIntegrity
+ final Set<KnnVectorsReader> genReaders =
+ Collections.newSetFromMap(new IdentityHashMap<KnnVectorsReader, Boolean>());
+ final LongArrayList vectorGens = new LongArrayList();
+
+ /**
+ * Creates a new producer that handles updated KNN vector fields.
+ *
+ * @param si commit point
+ * @param dir directory
+ * @param baseCoreReader the shared base (gen == -1) reader from {@link SegmentCoreReaders}; may
+ * be {@code null} if the core has no vector fields
+ * @param allInfos all fieldinfos including updated ones
+ * @param segKnnVectors per-gen reader cache
+ */
+ SegmentVectorsProducer(
+ SegmentCommitInfo si,
+ Directory dir,
+ KnnVectorsReader baseCoreReader,
+ FieldInfos allInfos,
+ SegmentKnnVectors segKnnVectors)
+ throws IOException {
+ this.fieldInfos = allInfos;
+ try {
+ for (FieldInfo fi : allInfos) {
+ if (fi.hasVectorValues() == false) {
+ continue;
+ }
+ long vectorGen = fi.getVectorGen();
+ if (vectorGen == -1) {
+ // served from the shared base core reader; not tracked here
+ readersByField.put(fi.number, baseCoreReader);
+ } else {
+ assert !vectorGens.contains(vectorGen);
Review Comment:
This mirrors `SegmentDocValuesProducer` exactly: `getKnnVectorsReader incRef()`s on a repeated gen, so per-add tracking stays refcount-balanced even if a gen were shared. So it's correct today, but maybe I could add an assertion guard here?
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on PR #16214:
URL: https://github.com/apache/lucene/pull/16214#issuecomment-4666575101
> Changing approximated search to exact search under the hood just because of an update feels wrong.
Thanks @iverase . It's deliberately consistent with `updateDocument`/`updateDocValues`, which also defer graph/secondary-structure work to the next merge. The intended workflow for a bulk re-embed is: update in place (cheap writes, 49 fields untouched) → one `forceMerge`/normal merge rebuilds the graph **once**, optimally. The exact-search window is bounded by a merge you were going to run anyway, and ANN is restored after it.
For workloads that can't tolerate that window, I'd add an **eager rebuild mode** as a follow-up: rebuild the field's graph at update time so approximate search stays intact on the updated segment, trading commit cost for unchanged read latency. (I had it implemented and pulled it to keep the default behavior aligned with the other in-place update APIs -- happy to bring it back behind an opt-in.)
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on PR #16214:
URL: https://github.com/apache/lucene/pull/16214#issuecomment-4647005988
This TOCTOU alert is a false positive. please dismiss it
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "iprithv (via GitHub)" <gi...@apache.org>.
iprithv commented on code in PR #16214:
URL: https://github.com/apache/lucene/pull/16214#discussion_r3374974915
##########
lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java:
##########
@@ -460,6 +510,187 @@ public long cost() {
}
}
+ /**
+ * Writes a new generation of KNN vector files for each field that has pending vector updates. For
+ * each such field we bump a new {@code vectorGen}, write {@code .vec/.vemf/.vex/.vem} files at
+ * that gen suffix where the vectors are the existing field vectors overlaid with the updates, and
+ * the HNSW graph is rebuilt eagerly (via the standard {@link KnnVectorsWriter} flush path). The
+ * base reader (gen == -1) plus other untouched fields are left in place. Vector analogue of
+ * {@link #handleDVUpdates}.
+ */
+ @SuppressWarnings("unchecked")
+ private synchronized void handleVectorUpdates(
+ FieldInfos infos,
+ Directory dir,
+ KnnVectorsFormat vectorsFormat,
+ final SegmentReader reader,
+ Map<Integer, Set<String>> fieldFiles,
+ long maxDelGen,
+ InfoStream infoStream,
+ boolean deferVectorGraphRebuild)
+ throws IOException {
+ for (Entry<String, List<KnnVectorFieldUpdates>> ent : pendingVectorUpdates.entrySet()) {
+ final String field = ent.getKey();
+ final List<KnnVectorFieldUpdates> updates = ent.getValue();
+ final List<KnnVectorFieldUpdates> updatesToApply = new ArrayList<>();
+ for (KnnVectorFieldUpdates update : updates) {
+ if (update.delGen <= maxDelGen) {
+ updatesToApply.add(update);
+ }
+ }
+ if (updatesToApply.isEmpty()) {
+ continue;
+ }
+
+ // Reject quantized formats: only the unquantized Lucene99 HNSW format is supported for
+ // in-place vector updates.
+ KnnVectorsFormat perFieldFormat = vectorsFormat;
+ if (vectorsFormat instanceof PerFieldKnnVectorsFormat perField) {
+ perFieldFormat = perField.getKnnVectorsFormatForField(field);
+ }
+ if (perFieldFormat instanceof Lucene99HnswVectorsFormat == false) {
+ throw new UnsupportedOperationException(
+ "in-place vector update is only supported for "
+ + Lucene99HnswVectorsFormat.class.getSimpleName()
+ + " but field ["
+ + field
+ + "] uses "
+ + perFieldFormat.getClass().getSimpleName());
+ }
+
+ final long nextVectorGen = info.getNextVectorGen();
+ final String segmentSuffix = Long.toString(nextVectorGen, Character.MAX_RADIX);
+ final IOContext updatesContext = IOContext.flush(new FlushInfo(info.info.maxDoc(), 0));
+
+ // Stamp the new vectorGen onto the (shared, cloned) FieldInfo so it is persisted by
+ // writeFieldInfosGen and picked up at read time. A single-field FieldInfos is handed to the
+ // vectors writer for this gen.
+ final FieldInfo fieldInfo = infos.fieldInfo(field);
+ assert fieldInfo != null && fieldInfo.hasVectorValues();
+ fieldInfo.setVectorGen(nextVectorGen);
+ final FieldInfos fieldInfos = new FieldInfos(new FieldInfo[] {fieldInfo});
+
+ // Choose the writer format for this generation. When deferring the graph rebuild we write
+ // only
+ // the flat vectors and skip building the HNSW graph: the gen's ".vex/.vem" carry an empty
+ // graph
+ // (vectorIndexLength == 0), so the reader falls back to an exact scan on this segment until
+ // the
+ // next merge rebuilds the graph using the codec's normal format. The graph build is
+ // suppressed
+ // by raising the "tiny segment" threshold above the segment size (so shouldCreateGraph() is
+ // always false for this write). We wrap it in a PerFieldKnnVectorsFormat so file naming and
+ // the
+ // per-field suffix attributes match what the (always per-field) reader reconstructs.
+ final KnnVectorsFormat writeFormat;
+ if (deferVectorGraphRebuild) {
+ final KnnVectorsFormat noGraphFormat =
+ new Lucene99HnswVectorsFormat(
+ Lucene99HnswVectorsFormat.DEFAULT_MAX_CONN,
+ Lucene99HnswVectorsFormat.DEFAULT_BEAM_WIDTH,
+ info.info.maxDoc() + 1);
+ writeFormat =
+ new PerFieldKnnVectorsFormat() {
+ @Override
+ public KnnVectorsFormat getKnnVectorsFormatForField(String f) {
+ return noGraphFormat;
+ }
+ };
+ } else {
+ writeFormat = vectorsFormat;
+ }
+
+ final TrackingDirectoryWrapper trackingDir = new TrackingDirectoryWrapper(dir);
+ final SegmentWriteState state =
+ new SegmentWriteState(
+ null, trackingDir, info.info, fieldInfos, null, updatesContext, segmentSuffix);
+
+ try (KnnVectorsWriter writer = writeFormat.fieldsWriter(state)) {
+ final KnnVectorFieldUpdates.Iterator mergedIterator =
+ KnnVectorFieldUpdates.mergedIterator(toIteratorArray(updatesToApply));
+ if (fieldInfo.getVectorEncoding() == VectorEncoding.FLOAT32) {
+ KnnFieldVectorsWriter<float[]> fieldWriter =
+ (KnnFieldVectorsWriter<float[]>) writer.addField(fieldInfo);
+ writeOverlayFloatVectors(reader, field, mergedIterator, fieldWriter);
+ } else {
+ KnnFieldVectorsWriter<byte[]> fieldWriter =
+ (KnnFieldVectorsWriter<byte[]>) writer.addField(fieldInfo);
+ writeOverlayByteVectors(reader, field, mergedIterator, fieldWriter);
+ }
+ writer.flush(info.info.maxDoc(), null);
+ writer.finish();
+ }
+
+ info.advanceVectorGen();
+ assert !fieldFiles.containsKey(fieldInfo.number);
+ fieldFiles.put(fieldInfo.number, trackingDir.getCreatedFiles());
+ }
+ }
+
+ private static KnnVectorFieldUpdates.Iterator[] toIteratorArray(
+ List<KnnVectorFieldUpdates> updatesToApply) {
+ KnnVectorFieldUpdates.Iterator[] subs =
+ new KnnVectorFieldUpdates.Iterator[updatesToApply.size()];
+ for (int i = 0; i < subs.length; i++) {
+ subs[i] = updatesToApply.get(i).iterator();
+ }
+ return subs;
+ }
+
+ /**
+ * Feeds the field writer, in docID order, every doc that currently has a float vector for {@code
+ * field}, substituting the updated vector when the merged update iterator has one for that doc.
+ */
+ private static void writeOverlayFloatVectors(
Review Comment:
If a user calls updateFloatVectorValue targeting a doc that doesn't actually have a vector for that field, wouldn't the update just silently disappears? The overlay loop only iterates docs that already have vectors, so any update targeting a doc without one gets skipped with no error.
##########
lucene/core/src/java/org/apache/lucene/index/KnnVectorFieldUpdates.java:
##########
@@ -0,0 +1,335 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+import static org.apache.lucene.search.DocIdSetIterator.NO_MORE_DOCS;
+
+import java.util.Comparator;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.util.ArrayUtil;
+import org.apache.lucene.util.PriorityQueue;
+
+/**
+ * Holds in-place KNN vector updates of a single field, for a set of documents within one segment.
+ * This is the vector analogue of {@link DocValuesFieldUpdates}: it accumulates (docID -> new
+ * vector) entries during resolution, sorts them by docID at {@link #finish()} (stable, so that the
+ * last update to a docID within a packet wins), and exposes an {@link Iterator} that the write path
+ * overlays onto the existing vectors.
+ *
+ * @lucene.experimental
+ */
+abstract class KnnVectorFieldUpdates {
+
+ final String field;
+ final VectorEncoding encoding;
+ final long delGen;
+ final int maxDoc;
+ final int dimension;
+ protected int[] docs = new int[8];
+ protected int size;
+ private boolean finished;
+
+ protected KnnVectorFieldUpdates(
+ int maxDoc, long delGen, String field, VectorEncoding encoding, int dimension) {
+ this.maxDoc = maxDoc;
+ this.delGen = delGen;
+ this.field = field;
+ this.encoding = encoding;
+ this.dimension = dimension;
+ }
+
+ /** Reserves a slot for the next doc and returns its ordinal. Subclasses store the value. */
+ protected final int reserve(int doc) {
+ if (finished) {
+ throw new IllegalStateException("already finished");
+ }
+ assert doc < maxDoc;
+ if (size == docs.length) {
+ docs = ArrayUtil.grow(docs, size + 1);
+ }
+ docs[size] = doc;
+ return size++;
+ }
+
+ boolean getFinished() {
+ return finished;
+ }
+
+ boolean any() {
+ return size > 0;
+ }
+
+ /** Freezes structures and stably sorts updates by docID (last write to a docID wins). */
+ final void finish() {
Review Comment:
This allocates a boxed Integer[] array and sorts it with a comparator, then copies everything into new arrays. DocValuesFieldUpdates.finish() does the same job in-place with IntroSorter and zero boxing, that's the established pattern here. No reason to diverge from it, especially since vectors are large so the extra copies are worse, not better.
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene94/Lucene94FieldInfosFormat.java:
##########
@@ -454,6 +458,8 @@ public void write(
output.writeVInt(fi.getVectorDimension());
output.writeByte((byte) fi.getVectorEncoding().ordinal());
output.writeByte(distFuncToOrd(fi.getVectorSimilarityFunction()));
+ // per-field KNN vector update generation (FORMAT_VECTOR_GEN+)
+ output.writeLong(fi.getVectorGen());
Review Comment:
The new vectorGen long is written unconditionally for every field, even plain text fields that will never have vectors. That's 8 extra bytes per field in every index. Could either only write it for fields with hasVectorValues(), or use a flag bit to signal presence, rather than bumping the format version and paying the cost everywhere.
##########
lucene/core/src/java/org/apache/lucene/index/BufferedUpdates.java:
##########
@@ -155,6 +159,15 @@ void addBinaryUpdate(BinaryDocValuesUpdate update, int docIDUpto) {
numFieldUpdates.incrementAndGet();
}
+ void addVectorUpdate(KnnVectorUpdate update, int docIDUpto) {
+ FieldUpdatesBuffer buffer =
+ vectorUpdates.computeIfAbsent(
Review Comment:
The first update for a field gets added twice. computeIfAbsent creates the buffer with the initial value already stored, then the next line unconditionally calls buffer.addUpdate() again with the same value. Compare with addNumericUpdate/addBinaryUpdate which have an if/else to avoid this. Harmless due to last-write-wins but wastes memory and looks like a bug.
##########
lucene/core/src/java/org/apache/lucene/index/SegmentVectorsProducer.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.Set;
+import org.apache.lucene.codecs.KnnVectorsReader;
+import org.apache.lucene.internal.hppc.IntObjectHashMap;
+import org.apache.lucene.internal.hppc.LongArrayList;
+import org.apache.lucene.search.AcceptDocs;
+import org.apache.lucene.search.KnnCollector;
+import org.apache.lucene.store.Directory;
+
+/**
+ * Encapsulates multiple per-generation {@link KnnVectorsReader}s as one reader when there are KNN
+ * vector updates. This is the vector analogue of {@link SegmentDocValuesProducer}.
+ *
+ * <p>Fields with no vector updates (vectorGen == -1) are served from the shared, immutable base
+ * reader held by {@link SegmentCoreReaders}; that base reader is never closed by this producer (its
+ * lifecycle is owned by the core). Fields with updates are served from per-gen readers tracked by
+ * {@link SegmentKnnVectors} via reference counting.
+ */
+class SegmentVectorsProducer extends KnnVectorsReader {
+
+ private final FieldInfos fieldInfos;
+ final IntObjectHashMap<KnnVectorsReader> readersByField = new IntObjectHashMap<>();
+ // distinct gen readers (excluding the base core reader) for checkIntegrity
+ final Set<KnnVectorsReader> genReaders =
+ Collections.newSetFromMap(new IdentityHashMap<KnnVectorsReader, Boolean>());
+ final LongArrayList vectorGens = new LongArrayList();
+
+ /**
+ * Creates a new producer that handles updated KNN vector fields.
+ *
+ * @param si commit point
+ * @param dir directory
+ * @param baseCoreReader the shared base (gen == -1) reader from {@link SegmentCoreReaders}; may
+ * be {@code null} if the core has no vector fields
+ * @param allInfos all fieldinfos including updated ones
+ * @param segKnnVectors per-gen reader cache
+ */
+ SegmentVectorsProducer(
+ SegmentCommitInfo si,
+ Directory dir,
+ KnnVectorsReader baseCoreReader,
+ FieldInfos allInfos,
+ SegmentKnnVectors segKnnVectors)
+ throws IOException {
+ this.fieldInfos = allInfos;
+ try {
+ for (FieldInfo fi : allInfos) {
+ if (fi.hasVectorValues() == false) {
+ continue;
+ }
+ long vectorGen = fi.getVectorGen();
+ if (vectorGen == -1) {
+ // served from the shared base core reader; not tracked here
+ readersByField.put(fi.number, baseCoreReader);
+ } else {
+ assert !vectorGens.contains(vectorGen);
Review Comment:
vectorGens is a LongArrayList (a list, not a set). If two fields ever share a generation in the future, you'd decRef the same gen-reader twice, which would close it prematurely. SegmentDocValuesProducer handles dvGens with deduplication awareness. Should at least deduplicate here or use a set to be safe.
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16214:
URL: https://github.com/apache/lucene/pull/16214#discussion_r3371224412
##########
lucene/core/src/java/org/apache/lucene/index/ReaderPool.java:
##########
@@ -210,11 +215,15 @@
~ // did was move the state to disk:
changed = true;
}
if (rld.writeFieldUpdates(
- directory, fieldNumbers, completedDelGenSupplier.getAsLong(), infoStream)) {
+ directory,
+ fieldNumbers,
+ completedDelGenSupplier.getAsLong(),
+ infoStream,
+ deferVectorGraphRebuild)) {
changed = true;
}
- if (rld.getNumDVUpdates() == 0) {
+ if (rld.getNumDVUpdates() == 0 && rld.getNumVectorUpdates() == 0) {
Review Comment:
## CodeQL / Time-of-check time-of-use race condition
This uses the state of [rld](1) which [is checked at a previous call](2). But these are not jointly synchronized.
[Show more details](https://github.com/apache/lucene/security/code-scanning/262)
##########
lucene/core/src/java/org/apache/lucene/index/ReaderPool.java:
##########
@@ -378,7 +399,7 @@
synchronized boolean anyDocValuesChanges() {
for (ReadersAndUpdates rld : readerMap.values()) {
// NOTE: we don't check for pending deletes because deletes carry over in RAM to NRT readers
- if (rld.getNumDVUpdates() != 0) {
+ if (rld.getNumDVUpdates() != 0 || rld.getNumVectorUpdates() != 0) {
Review Comment:
## CodeQL / Time-of-check time-of-use race condition
This uses the state of [rld](1) which [is checked at a previous call](2). But these are not jointly synchronized.
[Show more details](https://github.com/apache/lucene/security/code-scanning/263)
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "iprithv (via GitHub)" <gi...@apache.org>.
iprithv commented on code in PR #16214:
URL: https://github.com/apache/lucene/pull/16214#discussion_r3375059943
##########
lucene/core/src/java/org/apache/lucene/index/SegmentVectorsProducer.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.Set;
+import org.apache.lucene.codecs.KnnVectorsReader;
+import org.apache.lucene.internal.hppc.IntObjectHashMap;
+import org.apache.lucene.internal.hppc.LongArrayList;
+import org.apache.lucene.search.AcceptDocs;
+import org.apache.lucene.search.KnnCollector;
+import org.apache.lucene.store.Directory;
+
+/**
+ * Encapsulates multiple per-generation {@link KnnVectorsReader}s as one reader when there are KNN
+ * vector updates. This is the vector analogue of {@link SegmentDocValuesProducer}.
+ *
+ * <p>Fields with no vector updates (vectorGen == -1) are served from the shared, immutable base
+ * reader held by {@link SegmentCoreReaders}; that base reader is never closed by this producer (its
+ * lifecycle is owned by the core). Fields with updates are served from per-gen readers tracked by
+ * {@link SegmentKnnVectors} via reference counting.
+ */
+class SegmentVectorsProducer extends KnnVectorsReader {
+
+ private final FieldInfos fieldInfos;
+ final IntObjectHashMap<KnnVectorsReader> readersByField = new IntObjectHashMap<>();
+ // distinct gen readers (excluding the base core reader) for checkIntegrity
+ final Set<KnnVectorsReader> genReaders =
+ Collections.newSetFromMap(new IdentityHashMap<KnnVectorsReader, Boolean>());
+ final LongArrayList vectorGens = new LongArrayList();
+
+ /**
+ * Creates a new producer that handles updated KNN vector fields.
+ *
+ * @param si commit point
+ * @param dir directory
+ * @param baseCoreReader the shared base (gen == -1) reader from {@link SegmentCoreReaders}; may
+ * be {@code null} if the core has no vector fields
+ * @param allInfos all fieldinfos including updated ones
+ * @param segKnnVectors per-gen reader cache
+ */
+ SegmentVectorsProducer(
+ SegmentCommitInfo si,
+ Directory dir,
+ KnnVectorsReader baseCoreReader,
+ FieldInfos allInfos,
+ SegmentKnnVectors segKnnVectors)
+ throws IOException {
+ this.fieldInfos = allInfos;
+ try {
+ for (FieldInfo fi : allInfos) {
+ if (fi.hasVectorValues() == false) {
+ continue;
+ }
+ long vectorGen = fi.getVectorGen();
+ if (vectorGen == -1) {
+ // served from the shared base core reader; not tracked here
+ readersByField.put(fi.number, baseCoreReader);
+ } else {
+ assert !vectorGens.contains(vectorGen);
Review Comment:
vectorGens is a LongArrayList (a list, not a set). If two fields ever share a generation in the future, you'd decRef the same gen-reader twice, which would close it prematurely. SegmentDocValuesProducer handles dvGens with deduplication awareness. Should maybe add deduplicate here or use a set to be safe?
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin commented on code in PR #16214:
URL: https://github.com/apache/lucene/pull/16214#discussion_r3371340104
##########
lucene/core/src/java/org/apache/lucene/index/ReaderPool.java:
##########
@@ -210,11 +215,15 @@
~ // did was move the state to disk:
changed = true;
}
if (rld.writeFieldUpdates(
- directory, fieldNumbers, completedDelGenSupplier.getAsLong(), infoStream)) {
+ directory,
+ fieldNumbers,
+ completedDelGenSupplier.getAsLong(),
+ infoStream,
+ deferVectorGraphRebuild)) {
changed = true;
}
- if (rld.getNumDVUpdates() == 0) {
+ if (rld.getNumDVUpdates() == 0 && rld.getNumVectorUpdates() == 0) {
Review Comment:
Both CodeQL TOCTOU findings are within `synchronized` methods (`release`, `anyDocValuesChanges`) holding the `ReaderPool` monitor, and `getNumVectorUpdates()` is itself synchronized on the RLD. This mirrors the long-standing `getNumDVUpdates()` pattern in the same methods -- the new `getNumVectorUpdates()` checks add no new race. Proposing to dismiss these as by-design.
--
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] Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue [lucene]
Posted by "LantaoJin (via GitHub)" <gi...@apache.org>.
LantaoJin closed pull request #16214: Add in-place KNN vector field updates: IndexWriter.update{Float,Byte}VectorValue
URL: https://github.com/apache/lucene/pull/16214
--
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