shubhamvishu commented on code in PR #16092:
URL: https://github.com/apache/lucene/pull/16092#discussion_r3278674064
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsReader.java:
##########
@@ -212,7 +236,22 @@ public RandomVectorScorer getRandomVectorScorer(String
field, float[] target) th
fi.vectorDataOffset,
fi.vectorDataLength,
quantizedVectorData),
- target);
+ scoringTarget);
+ }
+
+ private HadamardRotation rotationFor(String field, int dimension) {
+ return rotations.computeIfAbsent(
+ field,
+ f ->
+ HadamardRotation.create(
+ dimension,
Lucene104ScalarQuantizedVectorsFormat.rotationSeed(f)));
+ }
+
+ private boolean isRotationEnabled(String field) {
+ FieldInfo info = fieldInfos.fieldInfo(field);
+ return info != null
+ && "true"
+
.equals(info.getAttribute(Lucene104ScalarQuantizedVectorsFormat.ROTATION_ENABLED_KEY));
Review Comment:
The initial version (1st commit) tried to retain it in the sandox module ->
then into core Codecs but it was writing the seed to the metadata of the field
and I had to bump the codec to 105. I honestly didn't wanted to bump the Codec
for this given case because we were writing a constant seed(or rotationEnabled
flag) into the index so I biased towards sharing the seed to query time via
`FieldInfos` and avoiding Codec bump since this was too we don't break the
backward compatibility and how simple it was in nature. I'm open to ideas
whichever we would want to choose or if there is a better way to share this
info(rotation enabled/disabled) without ideally avoid codec bump.
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsReader.java:
##########
@@ -197,6 +213,14 @@ public RandomVectorScorer getRandomVectorScorer(String
field, float[] target) th
if (fi == null) {
return null;
}
+ // Rotate the query vector to match the rotated stored vectors.
+ float[] scoringTarget = target;
+ if (isRotationEnabled(field) && target != null) {
+ HadamardRotation rotation = rotationFor(field, fi.dimension);
+ float[] rotated = new float[target.length];
+ rotation.rotate(target, rotated);
Review Comment:
`rotate` is a `O(d log d)` operation here. I ran the luceneutil without
forceMerge too but I don't have the Cohere results handy anymore(I can pull
those up again). Though I have the results with 4K dim embeddings in multi
segment index. Sharing those below for you reference. I didn't seem to regress
the latency as such. We can do more JMH benchmarking or whichever could give us
more confidence but so far it appears to be a cheap cost(Idk if the
quantization overhead was significant in past).
--
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]
