kaivalnp commented on code in PR #16710:
URL: https://github.com/apache/lucene/pull/16710#discussion_r4166943124
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupScalarQuantizedVectorsReader.java:
##########
@@ -462,9 +487,19 @@ 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();
+ // sides then resolve through the shared fieldOrdToGroupOrd translation.
The group's raw vectors
+ // are read directly from the group view (indexed by group ordinal, not
field ordinal) as
+ // float[] (FLOAT16 groups are inflated from short[]).
+ FloatVectorValues floatVectorValues;
+ if (fieldInfo.getVectorEncoding() == VectorEncoding.FLOAT16) {
+ DedupVectorValues dedupValues = (DedupVectorValues)
getFloat16VectorValues(fieldInfo.name);
+ floatVectorValues =
+ new Float16AsFloatVectorValues((Float16VectorValues)
dedupValues.getGroupView());
Review Comment:
This path reads `FieldEntry` again, also building unused quantized views. I
wonder if we could use `getRaw*VectorValues().getGroupView()` like before,
avoiding runtime casts too?
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupScalarQuantizedVectorsReader.java:
##########
@@ -447,11 +472,11 @@ public ScalarQuantizer getQuantizationState(String
fieldName) {
public CloseableRandomVectorScorerSupplier
getRandomVectorScorerSupplierForMerge(
FieldInfo fieldInfo, SegmentWriteState segmentWriteState) throws
IOException {
FieldEntry entry = fields.get(fieldInfo.name);
- if (entry == null || entry.fieldInfo().encoding() != FLOAT32) {
- // BYTE and FLOAT16 fields are stored raw only
+ if (entry == null || entry.fieldInfo().encoding().isFloatingPoint() ==
false) {
+ // Not a quantized field (missing, or e.g. BYTE stored raw only)
return null;
}
- FieldValues quantizedValues = getQuantizedVectorValues(entry);
+ FieldValues quantizedValues = (FieldValues)
getQuantizedVectorValues(fieldInfo.name);
Review Comment:
We can change the function signature to return `FieldValues` and avoid
casting.
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupScalarQuantizedVectorsReader.java:
##########
@@ -427,14 +428,38 @@ private Float16VectorValues
getFloat16VectorValues(FieldEntry entry) throws IOEx
entry.fieldInfo().fieldOrdToGroupOrdBitsPerValue());
}
+ private FieldValues getFloat16QuantizedVectorValues(FieldEntry entry) throws
IOException {
Review Comment:
I realized that
[`QuantizedByteVectorValues`](https://github.com/apache/lucene/blob/3970208e0d4d5e997a10baac816bd7d1d7693350/lucene/core/src/java/org/apache/lucene/util/quantization/QuantizedByteVectorValues.java#L27)
makes no assumptions about the vector encoding used to create the quantized
bytes, and allows scoring against either `float[]` or `short[]`.
I wonder if we can remove this function and
`DedupScalarQuantizedVectorValues.loadQuantizedFloat16` entirely? (replacing
with `loadQuantized` everywhere)
`loadQuantized` uses a float32 specific initializer to build the `fieldView`
[here](https://github.com/Pranshu-S/lucene/blob/8e198015173a8178813929db1701d43706f66ce2/lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupScalarQuantizedVectorValues.java#L103)
-- which could be replaced with
[`OffHeapScalarQuantizedVectorValues.load`](https://github.com/Pranshu-S/lucene/blob/8e198015173a8178813929db1701d43706f66ce2/lucene/core/src/java/org/apache/lucene/codecs/lucene104/OffHeapScalarQuantizedVectorValues.java#L195)
along the same lines (need to make it `public`).
We could also replace the type of `FieldValues.fieldView` from
`KnnVectorValues` -> a more concrete `OffHeapScalarQuantizedVectorValues` after
this?
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupUtil.java:
##########
@@ -212,6 +212,17 @@ static long hashBytes(byte[] bytes) {
return murmurhash3_x64_128(bytes, 0, bytes.length, GOOD_FAST_HASH_SEED)[0];
}
+ /**
+ * Inflates a float16 vector ({@code short[]}) into the reused {@code
float[]} buffer for the
+ * fp32-based scalar quantizer.
+ */
+ static float[] inflateFloat16(short[] float16Vector, float[] dest) {
Review Comment:
Can we add an assert for `float16Vector.length == dest.length`?
##########
lucene/sandbox/src/test/org/apache/lucene/sandbox/codecs/dedup/TestDedupScalarQuantizedVectorsFormat.java:
##########
@@ -399,6 +525,148 @@ public void testAllEncodingsMergeAndSearch() throws
Exception {
}
}
+ /**
+ * Every encoding survives a merge of FLOAT16 fields with graph building
forced (exercising the
+ * merge-time scorer suppliers, including the temporary query-vectors file
of asymmetric encodings
+ * and the FLOAT16 {@code short[]} -> {@code float[]} inflation on the raw
side) and searches
+ * sanely afterwards.
+ */
+ public void testFloat16AllEncodingsMergeAndSearch() throws Exception {
+ int dimension = 32;
+ int numDistinct = 20;
+ float[][] distinct = new float[numDistinct][];
+ for (int i = 0; i < numDistinct; i++) {
+ distinct[i] = randomVector(dimension);
+ }
+ for (ScalarEncoding encoding : ScalarEncoding.values()) {
+ for (VectorSimilarityFunction function :
VectorSimilarityFunction.values()) {
+ IndexWriterConfig config =
+ newIndexWriterConfig()
+ .setCodec(
+ TestUtil.alwaysKnnVectorsFormat(
+ new DedupHnswScalarQuantizedVectorsFormat(
+ encoding,
+ DEFAULT_MAX_CONN,
+ DEFAULT_BEAM_WIDTH,
+ 1,
+ null,
+ 0))); // always build graphs
+ try (Directory dir = newDirectory();
+ IndexWriter w = new IndexWriter(dir, config)) {
+ for (int segment = 0; segment < 2; segment++) { // duplicates across
segments
+ for (int i = 0; i < numDistinct; i++) {
+ Document doc = new Document();
+ float[] vector =
+ function == DOT_PRODUCT
+ ? VectorUtil.l2normalize(distinct[i].clone())
+ : distinct[i];
+ doc.add(new KnnFloat16VectorField("f", toFloat16(vector),
function));
+ w.addDocument(doc);
+ }
+ w.commit();
+ }
+ w.forceMerge(1); // exercises the merge-time graph construction
scorer
+
+ try (DirectoryReader reader = DirectoryReader.open(w)) {
+ LeafReader leafReader = getOnlyLeafReader(reader);
+ Float16VectorValues values =
leafReader.getFloat16VectorValues("f");
+ assertEquals(2 * numDistinct, values.size());
+ assertEquals(numDistinct, groupNumVectors(values)); //
de-duplicated across segments
+ assertGraphBuilt(leafReader, "f"); // threshold=0 forced the
merge-time graph build
+
+ float[] queryFloat =
+ function == DOT_PRODUCT ?
VectorUtil.l2normalize(distinct[0].clone()) : distinct[0];
+ short[] query = toFloat16(queryFloat);
+ TopDocs topDocs =
+ leafReader.searchNearestVectors(
+ "f",
+ query,
+ 2,
+ AcceptDocs.fromLiveDocs(null, leafReader.maxDoc()),
+ Integer.MAX_VALUE);
+ String context = "encoding=" + encoding + ", function=" + function;
+ assertEquals(context, 2, topDocs.scoreDocs.length);
+ // the two duplicates of the query vector share the (top) score
+ assertEquals(context, topDocs.scoreDocs[0].score,
topDocs.scoreDocs[1].score, 0f);
+ }
+ }
+ }
+ }
+ }
+
+ public void testFloat16SharedGroupAsymmetricMerge() throws Exception {
+ int dimension = 32;
+ int smallCount = 2;
+ int bigCount = 20;
+ float[][] small = new float[smallCount][];
+ float[][] big = new float[bigCount][];
+ for (int i = 0; i < smallCount; i++) {
+ small[i] = randomVector(dimension);
+ }
+ for (int i = 0; i < bigCount; i++) {
+ big[i] = randomVector(dimension);
+ }
+ for (ScalarEncoding encoding : ScalarEncoding.values()) {
+ if (encoding.isAsymmetric() == false) {
+ continue; // the EOF only reproduces on the asymmetric merge path
+ }
+ IndexWriterConfig config =
+ newIndexWriterConfig()
+ .setCodec(
+ TestUtil.alwaysKnnVectorsFormat(
+ new DedupHnswScalarQuantizedVectorsFormat(
+ encoding,
+ DEFAULT_MAX_CONN,
+ DEFAULT_BEAM_WIDTH,
+ 1,
+ null,
+ 0))); // always build graphs
+ try (Directory dir = newDirectory();
+ IndexWriter w = new IndexWriter(dir, config)) {
+ for (int segment = 0; segment < 2; segment++) { // duplicates across
segments
+ for (int i = 0; i < smallCount; i++) {
+ Document d = new Document();
+ d.add(new KnnFloat16VectorField("small", toFloat16(small[i]),
EUCLIDEAN));
+ w.addDocument(d);
+ }
+ for (int i = 0; i < bigCount; i++) {
+ Document d = new Document();
+ d.add(new KnnFloat16VectorField("big", toFloat16(big[i]),
EUCLIDEAN));
+ w.addDocument(d);
+ }
+ w.commit();
+ }
+ // the "small" field has only smallCount*2 doc-entries but shares a
group of
+ // smallCount+bigCount distinct vectors: merging it previously overran
its map.
Review Comment:
Context leak in comment?
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupScalarQuantizedVectorsReader.java:
##########
@@ -462,9 +488,19 @@ 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();
+ // sides then resolve through the shared fieldOrdToGroupOrd translation.
The group's raw vectors
+ // are read directly from the group view (indexed by group ordinal, not
field ordinal) as
+ // float[] (FLOAT16 groups are inflated from short[]).
+ FloatVectorValues floatVectorValues;
Review Comment:
Sorry about the confusion, but I mistakenly thought that
`Float16AsFloatVectorValues` was accessible here.
Your earlier version to use something like `FloatVectorSupplier
groupViewAsFloats` looks more suitable than duplicating that class just for
ordinal-based vector lookup.
--
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]
