[FIX] KNN search for 0 results with a filter fix - [MOD-5006] #3468
Conversation
GuyAv46
changed the title
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3468 +/- ##
==========================================
+ Coverage 82.71% 82.73% +0.01%
==========================================
Files 175 175
Lines 29946 29946
==========================================
+ Hits 24771 24776 +5
+ Misses 5175 5170 -5
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| @@ -424,7 +424,8 @@ IndexIterator *NewHybridVectorIterator(HybridIteratorParams hParams) { | |||
| hi->timeoutCtx = (TimeoutCtx){ .timeout = hParams.timeout, .counter = 0 }; | |||
| hi->runtimeParams.timeoutCtx = &hi->timeoutCtx; | |||
|
|
|||
| if (hParams.childIt == NULL) { | |||
| if (hParams.childIt == NULL || hParams.query.k == 0) { | |||
| // If there is no child iterator, or the query is going to return 0 results, we can use simple KNN. | |||
There was a problem hiding this comment.
If we know the query will return 0 results, is it not possible to optimize even further and short-circuit?
There was a problem hiding this comment.
only a bit more, but on 0 results it does not mean the entire query will return 0, and stuff still needs to be prepared. on the first read, we will return EOF
| @@ -424,7 +424,8 @@ IndexIterator *NewHybridVectorIterator(HybridIteratorParams hParams) { | |||
| hi->timeoutCtx = (TimeoutCtx){ .timeout = hParams.timeout, .counter = 0 }; | |||
| hi->runtimeParams.timeoutCtx = &hi->timeoutCtx; | |||
|
|
|||
| if (hParams.childIt == NULL) { | |||
| if (hParams.childIt == NULL || hParams.query.k == 0) { | |||
There was a problem hiding this comment.
This can be moved to an earlier phase, in NewVectorIterator function
There was a problem hiding this comment.
We actually can't, because today we have to evaluate all the iterators because they might yield a field that is needed somewhere else (we want to return 0 results and not cause a syntax error)
* handling k = 0 better * added test for k = 0
Addressing #3465, fixing a bug where we have a filter for the KNN query and we are looking for the top 0 results.
Because the internal heap, in this case, is empty, but its size is not smaller than
k, we tried overriding the "worst" result, but the pointer was null.This PR makes the hybrid reader iterator to choose simple KNN search when
k == 0, as we are going to return 0 results regardless of the child iterator, making such queries a bit faster (not reading the child iterator at all) and making the assumption thatk > 0in the case that there is a filter, valid.