fix(hybrid search results): fixed hybrid search by removing redundant…#731
Conversation
… slicing before filtering results were sliced and then filtered, causing incorrect document return and an upper-bounded count value. Now, filtering is done first to ensure accurate total count and correct document retrieval.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hi, can you please add some tests? Thank you |
In project, we already has test cases. |
… to the limit if we set a limit of 10, the total count of the results will not exceed 10, even if there are more matching documents. This means you don't get the actual total number of results that match your search criteria, but rather a maximum of the limit value.
Added the test cases. can you please check now |
|
@micheleriva are there any other changes required from my side to proceed further? let me know if needed. |
|
@micheleriva Hey, could you please review the PR changes? if any changes are needed please let me know |
| t.equal(page3.count, 20) | ||
|
|
||
| //ideally this has to show 2. not sure why it is like this | ||
| // t.equal(page1.count, 20) |
There was a problem hiding this comment.
The count is supposed to be equal to the total number of documents matching the query, not the paginated amount of documents returned, otherwise you might as well just use the hits.length property
There was a problem hiding this comment.
what about the second test case?. that count should also be 20
There was a problem hiding this comment.
I am confused. in the below example. the count is 2 even though the total count is 20.
https://codesandbox.io/p/sandbox/objective-sun-kzs96j?file=%2Fsrc%2Findex.ts
and also it is mentioned in the issue that there is an issue with
The count value is upper-bounded to the limit.
There was a problem hiding this comment.
My bad, I wasn't clear enough by:
total number of documents matching the query
I meant the total number of documents matching the query regardless of the offset / limit
…l to original as discussed
|
@nicolastoulemont Hey, reverted the test cases to their original values. can you please check now |
| }) | ||
| }) | ||
|
|
||
| t.test('should fix the issue realted #730', async (t) => { |
There was a problem hiding this comment.
NIT: Please write a better test name, describing in the code the intended behavior
| limit: 2, | ||
| offset: 2, | ||
| }); | ||
| const page4 = await search(db, { |
There was a problem hiding this comment.
NIT: Not sure the page4 is necessary here tbh
nicolastoulemont
left a comment
There was a problem hiding this comment.
LGTM overall, would love maintainers reviews tho
|
@micheleriva |
|
Hey! Sorry for the delay on getting back to you on this, this looks great! Merging it now |
|
Hi @rajeshj11 please give us one day to process this. |
yeah sure, no rush. Just want to know where it is blocked. |
|
@micheleriva may I know the status. |
/claim #730
/fixes #730
… slicing before filtering
results were sliced and then filtered, causing incorrect document return and an upper-bounded count value. Now, filtering is done first to ensure accurate total count and correct document retrieval.
demo video:
https://www.loom.com/share/63cc6cfee37c434596d79750977d5dff