Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove hits beyond max requested hit #5180

Merged
merged 3 commits into from
Jun 28, 2024
Merged

Conversation

trinity-1686a
Copy link
Contributor

Description

due to the 1st case of what's described in #3650 , it can happen that we return up to start_offset + 2 * max_hits documents instead of max_hits. This happens when a split first fails, and then succeed, but its result is just concatenated instead of doing a proper top-k.
The fix consist in dropping the tail hits at the same time as the hits that should be omitted due to start_offset are removed.

How was this PR tested?

tested with a modified s3 that errors on a fraction of requests. Without the patch, i often get too many results (and sometime a double error so no response), with the patch, i either get the right number of docs, or said error.

@trinity-1686a trinity-1686a requested a review from guilload June 28, 2024 07:49
Copy link

github-actions bot commented Jun 28, 2024

On SSD:

Average search latency is 0.995x that of the reference (lower is better).
Ref run id: 2374, ref commit: f4e5710
Link

On GCS:

Average search latency is 1.01x that of the reference (lower is better).
Ref run id: 2375, ref commit: f4e5710
Link

Copy link
Member

@guilload guilload left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be unit-tested? A mock leaf_search could first return an error and then a successful response.

@trinity-1686a trinity-1686a force-pushed the trinity/return-too-many-docs branch from 2a6e70b to 762dc73 Compare June 28, 2024 22:28
@trinity-1686a trinity-1686a merged commit 49c566d into main Jun 28, 2024
5 checks passed
@trinity-1686a trinity-1686a deleted the trinity/return-too-many-docs branch June 28, 2024 23:00
fulmicoton pushed a commit that referenced this pull request Jul 5, 2024
* also remove hits that are too many when removing skiped hits

* add mock-test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants