kaivalnp commented on code in PR #16710:
URL: https://github.com/apache/lucene/pull/16710#discussion_r4160279185
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupScalarQuantizedVectorsReader.java:
##########
@@ -461,11 +483,11 @@ public CloseableRandomVectorScorerSupplier
getRandomVectorScorerSupplierForMerge
// Asymmetric encodings compare query-encoded vectors against the stored
doc-encoded vectors.
// Write a query-encoded record per distinct vector of the group into a
temporary file; both
- // sides then resolve through the shared fieldOrdToGroupOrd translation.
- DedupVectorValues.FloatImpl rawValues = getRawFloatVectorValues(entry);
- FloatVectorValues groupView = rawValues.getGroupView();
Review Comment:
I wonder if we can avoid the new `FieldVectorsMergeView` class +
`fieldVectorsMergeView` function to simplify a bit: we just need a
`FloatVectorValues` here, the fp16 vector values can be wrapped by
[`Float16AsFloatVectorValues`](https://github.com/apache/lucene/blob/cb276351c10966e8afb3e234e9147d6145038e6a/lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsWriter.java#L1015),
which is basically what those methods do.
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupScalarQuantizedVectorValues.java:
##########
@@ -255,7 +306,23 @@ public VectorScorer scorer(float[] target) throws
IOException {
FieldValues copy = copy();
DocIndexIterator indexIterator = copy.iterator();
RandomVectorScorer vectorScorer =
vectorsScorer.getRandomVectorScorer(function, copy, target);
- boolean isDense = copy.fieldView instanceof
OffHeapFloatVectorValues.DenseOffHeapVectorValues;
+ boolean isDense =
+ copy.fieldView instanceof
OffHeapFloatVectorValues.DenseOffHeapVectorValues
+ || copy.fieldView instanceof
OffHeapFloat16VectorValues.DenseOffHeapVectorValues;
Review Comment:
I don't think we can build a scorer from a `float[]` target on a
`Float16VectorValues`?
Should the `instanceof` checks just include the correct one? (same below)
##########
lucene/sandbox/src/test/org/apache/lucene/sandbox/codecs/dedup/TestDedupScalarQuantizedVectorsFormat.java:
##########
Review Comment:
Can you add a `FLOAT16` variant for this too?
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupScalarQuantizedVectorValues.java:
##########
@@ -328,4 +383,88 @@ public VectorScorer rescorer(float[] target) throws
IOException {
return rawValues.rescorer(target);
}
}
+
+ /**
+ * FLOAT16 analogue of {@link RawAndQuantizedValues}: exposes a field's raw
de-duplicated {@code
+ * short[]} vectors for full-fidelity readback, while {@link
#scorer(short[])} scores against the
+ * shared quantized view (the {@code short[]} target is inflated to {@code
float[]} to match the
+ * data-blind quantizer). Mirrors the FLOAT16 handling in {@code
+ * Lucene104ScalarQuantizedVectorsReader}.
+ */
+ static final class Float16RawAndQuantizedValues extends Float16VectorValues
+ implements DedupVectorValues {
+ private final DedupVectorValues.Float16Impl rawValues;
+ private final FieldValues quantizedValues;
+
+ Float16RawAndQuantizedValues(
+ DedupVectorValues.Float16Impl rawValues, FieldValues quantizedValues) {
+ this.rawValues = rawValues;
+ this.quantizedValues = quantizedValues;
+ }
+
+ FieldValues getQuantizedValues() {
+ return quantizedValues;
+ }
+
+ @Override
+ public KnnVectorValues getGroupView() {
+ return rawValues.getGroupView();
+ }
+
+ @Override
+ public FieldOrdToGroupOrd getFieldOrdToGroupOrd() {
+ return rawValues.getFieldOrdToGroupOrd();
+ }
+
+ @Override
+ public int dimension() {
+ return rawValues.dimension();
+ }
+
+ @Override
+ public int size() {
+ return rawValues.size();
+ }
+
+ @Override
+ public int ordToDoc(int ord) {
+ return rawValues.ordToDoc(ord);
+ }
+
+ @Override
+ public Bits getAcceptOrds(Bits acceptDocs) {
+ return rawValues.getAcceptOrds(acceptDocs);
+ }
+
+ @Override
+ public void prefetch(int[] ordsToPrefetch, int numOrds) throws IOException
{
+ rawValues.prefetch(ordsToPrefetch, numOrds);
+ }
+
+ @Override
+ public short[] vectorValue(int ord) throws IOException {
+ return rawValues.vectorValue(ord);
+ }
+
+ @Override
+ public DocIndexIterator iterator() {
+ return rawValues.iterator();
+ }
+
+ @Override
+ public Float16RawAndQuantizedValues copy() throws IOException {
+ return new Float16RawAndQuantizedValues(rawValues.copy(),
quantizedValues.copy());
+ }
+
+ @Override
+ public VectorScorer scorer(short[] target) throws IOException {
+ float[] inflated = DedupUtil.inflateFloat16(target, new
float[target.length]);
Review Comment:
I wonder why this wasn't hit in tests, I'm guessing it's because of
`tinySegmentsThreshold` that the merge paths aren't exercised at all. Can you
update the test so that graphs are always built, and all paths are covered?
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupScalarQuantizedVectorsScorer.java:
##########
@@ -41,10 +41,14 @@ final class DedupScalarQuantizedVectorsScorer extends
DedupFlatVectorsScorer {
super(QUANTIZED_SCORER);
}
- /** Full-precision views resolve to their quantized values for scoring. */
+ /** Full-precision views (float32 or float16) resolve to their quantized
values for scoring. */
@Override
protected KnnVectorValues unwrap(KnnVectorValues vectorValues) {
- if (vectorValues instanceof RawAndQuantizedValues rawAndQuantized) {
+ if (vectorValues instanceof Float32RawAndQuantizedValues rawAndQuantized) {
+ return rawAndQuantized.getQuantizedValues();
+ }
+ if (vectorValues
+ instanceof
DedupScalarQuantizedVectorValues.Float16RawAndQuantizedValues rawAndQuantized) {
Review Comment:
nit: can you keep qualifiers consistent with `Float32RawAndQuantizedValues`?
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupScalarQuantizedVectorsReader.java:
##########
Review Comment:
We also need to support `FLOAT16` [like the "vanilla"
reader](https://github.com/apache/lucene/blob/cb276351c10966e8afb3e234e9147d6145038e6a/lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsReader.java#L463-L481).
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupScalarQuantizedVectorValues.java:
##########
@@ -338,4 +407,85 @@ public VectorScorer rescorer(float[] target) throws
IOException {
return rawValues.rescorer(target);
}
}
+
+ /**
+ * FLOAT16 analogue of {@link Float32RawAndQuantizedValues}: exposes a
field's raw de-duplicated
Review Comment:
nit: the `analogue` comment is good to explain changes, but can we keep this
in-line with `Float32RawAndQuantizedValues`?
--
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]
