Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Fix distincted exhaustive hits #729

Merged
merged 2 commits into from
Dec 8, 2022
Merged

Fix distincted exhaustive hits #729

merged 2 commits into from
Dec 8, 2022

Conversation

ManyTheFish
Copy link
Member

@ManyTheFish ManyTheFish commented Dec 7, 2022 •
edited

This PR changes the name and behavior of bucket_candidates:

  • bucket_candidates become initial_candidates that is less confusing
  • initial_candidates is no more a simple RoaringBitmap but an enum allowing us to precise if the candidates are exhaustive or not
  • this enum ensures that any modification is allowed only if the candidates are not already exhaustive.

The bug occurred because initial_candidates are modified during the bucket sort allowing the estimation to be more and more precise along the search, and this was an issue when the initial_candidates were already exhaustive, now, if candidates are exhaustive, then no modifications are made.

@ManyTheFish ManyTheFish added the bug Something isn't working label Dec 8, 2022
@ManyTheFish ManyTheFish marked this pull request as ready for review December 8, 2022 08:41
@ManyTheFish ManyTheFish added the no breaking The related changes are not breaking (DB nor API) label Dec 8, 2022
Kerollmops
Kerollmops approved these changes Dec 8, 2022
View reviewed changes
Copy link
Member

@Kerollmops Kerollmops left a comment •
edited

Choose a reason for hiding this comment

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

Nice changes!
I was wondering why there were that many changes but you needed to change the whole set of ranking rules 😄

bors merge

@bors
Copy link
Contributor

bors bot commented Dec 8, 2022

Build succeeded:

@bors bors bot merged commit 1f1beae into main Dec 8, 2022
@bors bors bot deleted the fix-distincted-exhaustive-hits branch December 8, 2022 09:48
This was referenced Dec 8, 2022
Closed
Merged
bors bot added a commit that referenced this pull request Dec 8, 2022
@bors @loiclec @ManyTheFish
Merge #731
1582b96 
731: Bug fixes meili v0.30.2 r=curquiza a=curquiza

⚠️  `@ManyTheFish` and `@loiclec,` check all your commits from #727 and #729 are here

Co-authored-by: Loïc Lecrenier <loic.lecrenier@me.com>
Co-authored-by: ManyTheFish <many@meilisearch.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Kerollmops Kerollmops Kerollmops approved these changes

Assignees
No one assigned
Labels
bug Something isn't working no breaking The related changes are not breaking (DB nor API)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ManyTheFish @Kerollmops