jtibshirani commented on code in PR #932:
URL: https://github.com/apache/lucene/pull/932#discussion_r890545274
##########
lucene/core/src/java/org/apache/lucene/search/KnnVectorQuery.java:
##########
@@ -225,6 +225,11 @@ public BitSetIterator getIterator(int contextOrd) {
return new BitSetIterator(bitSets[contextOrd], cost[contextOrd]);
}
+ public void setBitSet(BitSet bitSet, int cost) {
+ bitSets[ord] = bitSet;
Review Comment:
Thank you @kaivalnp , these results are insightful. I like the direction of
the last change "Modifying Collection", it's clearly valuable to be able to
access the cached or precomputed `BitSet` directly.
Stepping back, I am not clear on we are hoping to benchmark with these
changes to `KnnGraphTester`. There is indeed overhead from copying the
`BitSet`, but this is just how the query works today (until we fix it!) I'm not
sure we should add a special test-only code path so that `KnnGraphTester` can
work around this limitation.
What would you think of this plan?
* Spin off a separate issue around removing overhead from copying `BitSet`
when the query is cached or precomputed. Maybe we'll end up with something
similar to your change where we access the iterator directly.
* Either hold off on this PR until that overhead is addressed, or merge it
but without a special workaround to prevent copying. To unblock any testing you
could fork `KnnGraphTester` locally or `KnnVectorQuery` to add a workaround?
--
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: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
