Updating Dense#intoBitSet to take into account the provided bitset if upTo equals to NO_MORE_DOCS #14882
Conversation
github-project-automation
bot
moved this to Open
in OpenSearch Lucene & Core Performance Tracking
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
| doc.add( | ||
| new KnnFloatVectorField( | ||
| "vector", new float[] {i, i * 2}, VectorSimilarityFunction.DOT_PRODUCT)); | ||
| if (i % 2 == 0) { |
There was a problem hiding this comment.
Tried adding a reproducible case so that we can enforce DENSE for IndexedDISI. Happy to discuss alternative approaches to ensure this.
| int destFrom = disi.doc - offset; | ||
| int disiTo = upTo == DocIdSetIterator.NO_MORE_DOCS ? bitSet.length() : upTo; |
There was a problem hiding this comment.
Maybe use Math.min(upTo, bitSet.length()) to be more robust.
There was a problem hiding this comment.
Updated in e03699a.
| @@ -276,6 +277,40 @@ public void testDocAndScoreQueryBasics() throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| @Nightly | |||
| public void testKnnQueryWithFiltering() throws IOException { | |||
There was a problem hiding this comment.
I wonder if TestIndexedDISI is a better place to test this?
There was a problem hiding this comment.
Added another one in TestIndexedDisi in e03699a, that I believe captures the issue. Should we remove this one altogether?
There was a problem hiding this comment.
+1 to remove this, the test in TestIndexedDISI looks good enough to me.
…computation for disiTo to be more robust
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
| @@ -276,6 +277,40 @@ public void testDocAndScoreQueryBasics() throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| @Nightly | |||
| public void testKnnQueryWithFiltering() throws IOException { | |||
There was a problem hiding this comment.
+1 to remove this, the test in TestIndexedDISI looks good enough to me.
|
Maybe add a CHANGES entry under 10.3.0 as well |
github-project-automation
bot
moved this from Open
to Merged
in OpenSearch Lucene & Core Performance Tracking
|
During the backport process, I noticed many tests failing. I wonder if CI didn't fully exercise paths here, or there were other changes since this PR was opened. On 10x with the backport, I ran into things like:
|
The tests are randomized, it can happen that a PR run doesn't hit something that will surface later. I think it's ok. So is reverting to figure out what's going on. |
@dweiss for sure, its random. But it failed immediately when I tested locally. It just surprised me :). I wonder if there was other work done in this area recently and this PR just needed to be updated to ensure it was fully tested. |
|
I think that the issue seems to be with the following as the above computation does not take into account the initial seems to address all failures (had 50 full test successful runs). Would you suggest to correct only when |
When specifying a
knnquery with a custom filter, it is possible to be met with the following IOOB exceptionThis seems to happen because when creating a
FixedBitSetwithmaxDocsize inAbstractKnnVectorQuery#createBitSetwe end up providing an
upTovalue ofDocIdSetIterator.NO_MORE_DOCSthrough theor. So, when we reachFixedBitSet.orRangewe try to verify thedestbitset based onBLOCK_SIZEsize, even though the actual size is usually less than that, causing the above IIOB exception.The issue is reproducible for
IndexedDISI.Method.Dense#intoBitSetWithinBlockonly, and not forSparse.In this PR, we update the upper bound provided to
FixedBitSet.orRangeto take into account thebitSet.length()ifupTo == NO_MORE_DOCS. If this seems too broad, we can narrow down the fix specifically to theAbstractKnnVectorQuerycase, by providing a well definedupTotoor.