jimczi commented on code in PR #16734:
URL: https://github.com/apache/lucene/pull/16734#discussion_r4156324849
##########
lucene/core/src/java/org/apache/lucene/codecs/KnnFieldVectorsWriter.java:
##########
@@ -36,6 +37,28 @@ protected KnnFieldVectorsWriter() {}
*/
public abstract void addValue(int docID, T vectorValue) throws IOException;
+ /**
+ * Add {@code values.size()} vectors for the consecutive doc IDs {@code
[firstDocID, firstDocID +
+ * values.size())}. {@code firstDocID} must be greater than every doc ID
added so far, and every
+ * vector has exactly {@code values.dimension()} elements, which matches the
field's dimension.
+ * Implementations must consume exactly {@code values.size()} vectors.
+ *
+ * <p>The cursor may throw while it is being consumed, for example when a
vector fails validation.
+ * In that case the documents of the whole batch are marked as deleted, but
the writer must remain
+ * in a consistent state: every doc ID it has recorded must have its vector.
+ *
+ * <p>The default implementation calls {@link #addValue} once per vector.
Override for a more
+ * efficient bulk path.
+ *
+ * @lucene.experimental
+ */
+ public void addDenseValues(int firstDocID, VectorValuesCursor<T> values)
throws IOException {
Review Comment:
Nothing overrides this yet, so the dense path still copies one vector at a
time. Could we add the default flat writer as a first implementer in this PR?
It would show that the `fill` signature is the right one before other codecs
start to build on it. The HNSW and scalar quantized field writers would also
need to forward to their flat delegate, otherwise an override there is never
reached.
##########
lucene/core/src/java/org/apache/lucene/codecs/KnnFieldVectorsWriter.java:
##########
@@ -36,6 +37,28 @@ protected KnnFieldVectorsWriter() {}
*/
public abstract void addValue(int docID, T vectorValue) throws IOException;
+ /**
+ * Add {@code values.size()} vectors for the consecutive doc IDs {@code
[firstDocID, firstDocID +
+ * values.size())}. {@code firstDocID} must be greater than every doc ID
added so far, and every
+ * vector has exactly {@code values.dimension()} elements, which matches the
field's dimension.
+ * Implementations must consume exactly {@code values.size()} vectors.
+ *
+ * <p>The cursor may throw while it is being consumed, for example when a
vector fails validation.
+ * In that case the documents of the whole batch are marked as deleted, but
the writer must remain
Review Comment:
Can we say here that the writer should only record doc IDs after `fill`
returns? Validation runs after the copy, so when `fill` throws the buffer
already holds the bad vectors.
##########
lucene/core/src/java/org/apache/lucene/document/column/VectorValuesCursor.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.document.column;
+
+import java.lang.reflect.Array;
+
+/**
+ * A values cursor over a dense {@link VectorColumn}. The cursor produces
exactly {@link #size()}
+ * vectors of {@link #dimension()} elements each, for consecutive batch-local
doc-ids starting at 0.
+ * Vectors are consumed one at a time with {@link #next()} or in bulk with
{@link #fill}.
+ *
+ * <p>The type parameter {@code T} is the vector array type: {@code float[]},
{@code short[]}
+ * (float16 bits) or {@code byte[]}, matching the column's {@link
VectorColumn} type parameter.
+ *
+ * <p>Implementations must throw an exception if more than {@link #size()}
vectors are consumed
+ * across {@link #next()} and {@link #fill}.
+ *
+ * @param <T> the vector array type
+ * @lucene.experimental
+ */
+public abstract class VectorValuesCursor<T> {
+
+ private final int size;
+ private final int dimension;
+
+ /**
+ * Creates a cursor that will produce exactly {@code size} vectors of {@code
dimension} elements,
+ * one per batch-local doc-id in {@code [0, size)}. Both are fixed for the
cursor's lifetime:
+ * {@code size} must equal the dense column's {@code numDocs} and {@code
dimension} must equal the
+ * field type's {@code vectorDimension()}.
+ *
+ * <p>Lucene's internal indexing paths will not consume past {@code size}
across {@link #next()}
+ * and {@link #fill}. Defensive throws on overrun are still encouraged to
catch misuse from
+ * external callers.
+ */
+ protected VectorValuesCursor(int size, int dimension) {
+ if (size < 0) {
+ throw new IllegalArgumentException("size must be >= 0; got " + size);
+ }
+ if (dimension <= 0) {
+ throw new IllegalArgumentException("dimension must be > 0; got " +
dimension);
+ }
+ this.size = size;
+ this.dimension = dimension;
+ }
+
+ /** Total number of vectors this cursor will produce. */
+ public final int size() {
+ return size;
+ }
+
+ /** Number of elements in each vector. */
+ public final int dimension() {
+ return dimension;
+ }
+
+ /**
+ * Returns the next vector, which must have exactly {@link #dimension()}
elements. The returned
+ * array may be reused by the cursor, so it is only valid until the next
call to {@link #next()}
+ * or {@link #fill}. Must not be called more than {@link #size()} times.
+ */
+ public abstract T next();
+
+ /**
+ * Bulk-fill the next {@code count} vectors into {@code dst}, advancing the
cursor by {@code
+ * count}. The vectors are written in flat row-major order: {@code count *
dimension()} elements
+ * starting at element {@code dstOffset}, so vector {@code i} occupies
{@code [dstOffset + i *
+ * dimension(), dstOffset + (i + 1) * dimension())}. Combined {@link
#next()} and {@code fill}
+ * calls must not consume more than {@link #size()} vectors.
+ *
+ * <p>The default implementation calls {@link #next()} in a loop, checks
that each vector has
+ * {@link #dimension()} elements, and copies it with {@link
System#arraycopy}. Override to provide
+ * a more efficient bulk fill, for example a single {@link System#arraycopy}
from a flat backing
+ * array; such overrides take responsibility for every vector having {@link
#dimension()}
+ * elements, since the indexing chain only validates the values written to
{@code dst}.
+ *
+ * @throws IllegalArgumentException if a vector returned by {@link #next()}
does not have {@link
+ * #dimension()} elements
+ */
+ public void fill(T dst, int dstOffset, int count) {
Review Comment:
This assumes the writer stores vectors in a heap array. A writer that
buffers in byte buffers or off-heap would need a scratch array to use it. Maybe
that is fine, but it is hard to tell without a real implementer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
