Repository navigation
Add bulk vector path to columnar addBatch - #16734
mayya-sharipova wants to merge 3 commits into
Conversation
Builds on apache#16708, which added validation. Vector columns declared DENSE now go through a bulk path, and those checks come along with it via a validating cursor, so addBatch keeps rejecting what addDocument rejects. Column API: - Add VectorValuesCursor<T>, a values cursor for DENSE columns modeled on LongValuesCursor. - Add VectorColumn.values(). Indexing chain: - Vector columns declared DENSE are handed to the writer in a single addDenseValues call. The dense count and the cursor's dimension are checked once per batch instead of per vector. - ValidatingVectorValuesCursor runs the apache#16708 checks as the writer consumes vectors. - ColumnValidation gains offset-based variants of the value checks and checkVectorCursorDimension. Codec API: - Add KnnFieldVectorsWriter.addDenseValues, which defaults to calling addValue once per vector.
jimczi
left a comment
jimczi
left a comment
There was a problem hiding this comment.
This looks good. The cursor follows the same pattern as the long columns and keeping validation inside the cursor makes sense to me. My main question is about landing the codec API without a writer that uses it, see the inline comment.
| * | ||
| * @lucene.experimental | ||
| */ | ||
| public void addDenseValues(int firstDocID, VectorValuesCursor<T> values) throws IOException { |
There was a problem hiding this 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.
There was a problem hiding this comment.
I added the default flat writer as a first implementer, and the HNSW and SQ field writers now forward to their flat delegate.
But as you see there is no much optimization done yet: filling one vector at a time. The plan is to move the flat writer to paged storage in a follow-up, where addDenseValues fills a whole page per fill call. This will be in the follow up PR.
| * 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 |
There was a problem hiding this 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.
| * @throws IllegalArgumentException if a vector returned by {@link #next()} does not have {@link | ||
| * #dimension()} elements | ||
| */ | ||
| public void fill(T dst, int dstOffset, int count) { |
There was a problem hiding this 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.
There was a problem hiding this comment.
For now, just heap-array. In the follow-up work, we can decide how to organize paged-storage: byte buffers or off-heap.
Addresses review feedback: give addDenseValues a real implementer so the fill signature is exercised before other codecs build on it. - Lucene99FlatVectorsWriter's default field writer fills each vector directly into a new array, one copy per vector, and records doc IDs and vectors only after every fill has returned. - Lucene99HnswVectorsWriter and Lucene104ScalarQuantizedVectorsWriter forward to their flat delegate, then add the new nodes to the graph or accumulate the new vectors for the centroid. - KnnFieldVectorsWriter.addDenseValues javadoc: record doc IDs only after fill returns, and forward the method from wrapping writers.
2 participants
Builds on #16708, which added validation. Vector columns declared DENSE now go through a bulk path, and those checks come along with it via a validating cursor, so addBatch keeps rejecting what addDocument rejects.
Column API:
Indexing chain:
Codec API: