mikemccand commented on code in PR #16092:
URL: https://github.com/apache/lucene/pull/16092#discussion_r3362073505
##########
lucene/core/src/java/org/apache/lucene/search/KnnFloatVectorQuery.java:
##########
@@ -95,6 +98,26 @@ public KnnFloatVectorQuery(
String field, float[] target, int k, Query filter, KnnSearchStrategy
searchStrategy) {
super(field, k, filter, searchStrategy);
this.target = VectorUtil.checkFinite(Objects.requireNonNull(target,
"target"));
+ this.targetPreRotated = false;
+ }
+
+ private boolean targetPreRotated;
+
+ @Override
+ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
+ if (targetPreRotated == false) {
+ FieldInfo fi =
+
FieldInfos.getMergedFieldInfos(indexSearcher.getIndexReader()).fieldInfo(field);
+ if (fi != null
+ && fi.getVectorDimension() == target.length
+ &&
"true".equals(fi.getAttribute(RotationAwareKnnVectorsFormat.ROTATION_ENABLED_KEY)))
{
+ float[] rotated = new float[target.length];
+ HadamardRotation.forDimension(fi.getVectorDimension()).rotate(target,
rotated);
Review Comment:
> I'm thinking if for starter we should keep the rotation per segment and
keep this change restricted to rotation like a recall vs latency tradeoff.
+1 for PnP ("progress not perfection"). New features should rarely be
blocked because of performance, when they are opt-in (not the default) -- that
can always be iteratively improved. Still, it's always good to debate
approaches / APIs. We should anyways mark the new APIs `@lucene.experimental`
so we are free to iterate them even across Lucene releases.
> We could follow with more ideas on how we could clawback the latency.
I'm still confused -- why is latency a problem? It amortizes to 0% as the
index gets bigger and bigger? (Because it'd be a fixed per-segment cost, like
the cost of seeking the terms dict to locate postings for that segment). Also,
"matrix times vector" is such an insanely well optimized function these days...
surely we could (eventually) SIMD-ify it? If I read the JMH results right,
it's ~6.5 microseconds per rotate for 1K vector, say times 30 segments, 200
microseconds (0.2 msec). That's fine for starters -- it's opt-in -- optimize
later!
> Another good thing of doing it per segment would be that way the whole
rotation logic would be retained in the KNN vector format and not spilling into
Query implementations etc so we won't be unintentionally breaking any current
use cases but providing user an way to improve recall at the cost of some
latency overhead for starters and later optimize (keeping this change focussed
on capability addition).
+1 for that simplicity.
> Perhaps one way to catch such cases is to randomly wrap the KNN format
being tested with a rotating one in
[BaseKnnVectorsFormatTestCase](https://github.com/apache/lucene/blob/6ce7f9cfe2006bba4271b92078ef084a84d86d12/lucene/test-framework/src/java/org/apache/lucene/tests/index/BaseKnnVectorsFormatTestCase.java#L138-L140)
(or if we can accept slower tests: explicitly test everything with and without
rotation).
Big +1 for that -- random testing is powerful, given enough time all bugs
are shallow! (The monkey Shakespeare typewriter thing, remixed with Linus' Law
heh)
--
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]
