jimczi commented on code in PR #16647:
URL: https://github.com/apache/lucene/pull/16647#discussion_r3949119731
##########
lucene/test-framework/src/java/org/apache/lucene/tests/index/BaseKnnVectorsFormatTestCase.java:
##########
@@ -2499,10 +2588,41 @@ protected void assertOffHeapByteSize(LeafReader r,
String fieldName) throws IOEx
protected static int getNumVectors(KnnVectorsReader reader, FieldInfo
fieldInfo)
throws IOException {
+ return reader.getVectorCount(fieldInfo);
+ }
+
+ protected void assertVectorCountMatchesVectorValuesSize(LeafReader
leafReader)
+ throws IOException {
+ if (leafReader instanceof CodecReader codecReader) {
+ KnnVectorsReader vectorsReader = codecReader.getVectorReader();
+ for (FieldInfo fieldInfo : leafReader.getFieldInfos()) {
+ if (fieldInfo.getVectorDimension() <= 0) {
+ continue;
+ }
+ KnnVectorsReader fieldReader =
vectorsReader.unwrapReaderForField(fieldInfo.name);
Review Comment:
Unwrapping here means the `PerFieldKnnVectorsFormat` override is never
exercised, and that's the one every caller reaches via
`CodecReader.getVectorReader()`. Same for the `SlowCodecReaderWrapper` /
`SlowCompositeCodecReaderWrapper` / `SortingCodecReader` overrides. Asserting
both the unwrapped and non-unwrapped reader would cover them.
##########
lucene/backward-codecs/src/java/org/apache/lucene/backward_codecs/lucene90/Lucene90HnswVectorsReader.java:
##########
@@ -331,6 +331,11 @@ public Map<String, Long> getOffHeapByteSize(FieldInfo
fieldInfo) {
return Map.ofEntries(raw, graph);
}
+ @Override
+ public int getVectorCount(FieldInfo fieldInfo) {
+ return getFieldEntry(fieldInfo.name).ordToDoc.length;
Review Comment:
`FieldEntry.size()` (line 384) returns exactly this - use it, like the
91/92/94/95 overrides do.
##########
lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsReader.java:
##########
@@ -185,6 +185,27 @@ public KnnVectorsReader getMergeInstance() throws
IOException {
*/
public void finishMerge() throws IOException {}
+ /**
+ * Returns the number of indexed vectors for the given field in this segment.
+ *
+ * <p>This has the same meaning as {@code get*VectorValues(field).size()}:
the number of vector
+ * ordinals stored for {@code fieldInfo} in this segment.
+ *
+ * <p>Standard vector formats read this from segment metadata without
opening vector values. The
+ * default implementation opens vector values as a fallback; callers that
need to avoid that I/O
+ * should use a reader that overrides this method.
+ *
+ * @param fieldInfo the fieldInfo
+ * @return the number of indexed vectors for the field
+ */
+ public int getVectorCount(FieldInfo fieldInfo) throws IOException {
Review Comment:
Worth pinning down what happens for a field that isn't vector-indexed -
implementations disagree today: `IllegalArgumentException` in most, NPE in
`PerFieldKnnVectorsFormat`, `0` in `KnnVectorsFormat.EMPTY`. `@throws
IllegalArgumentException` to match `getFloatVectorValues` would do it.
Also worth stating here that opening vector values may do I/O and prefetch
by design - that's what justifies this method existing, and it's the same
reason doc values grew `DocValuesSkipper` instead of a cheaper iterator.
##########
lucene/core/src/java/org/apache/lucene/index/SlowCodecReaderWrapper.java:
##########
@@ -213,6 +213,14 @@ public Map<String, Long> getOffHeapByteSize(FieldInfo
fieldInfo) {
return vectorsReader.getOffHeapByteSize(fieldInfo);
}
+ @Override
+ public int getVectorCount(FieldInfo fieldInfo) throws IOException {
Review Comment:
This is only reachable from the non-`CodecReader` branch of `wrap()`, i.e.
arbitrary leaf readers, but `segmentReader(reader)` throws on anything that
doesn't unwrap to a `SegmentReader`. With a `ParallelLeafReader` over a segment:
```
values.size() -> 10
getVectorCount(fi) -> java.lang.AssertionError: unexpected reader
[ParallelLeafReader(_1(11.0.0):c10)]
```
The inherited default delegates to `reader.getXVectorValues(field).size()`,
which is correct here and can't fail. Can we drop the override, or fall back to
`super.getVectorCount(fieldInfo)` when the leaf doesn't unwrap?
(`getOffHeapByteSize` above has the same issue - pre-existing, separate.)
--
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]
