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

Fix typo initial candidates computation #737

Merged
merged 2 commits into from
Dec 13, 2022
Merged

Conversation

ManyTheFish
Copy link
Member

@ManyTheFish ManyTheFish commented Dec 12, 2022

When Typo criterion was after a different criterion than Words and the previous criterion wasn't returning any candidates at the first iteration of the bucket sort, then the initial_candidates were lost.

Now, Typoensure to keep the initial_candidates between iterations.

related to meilisearch/meilisearch#3200 (comment)
related to meilisearch/meilisearch#3228

@ManyTheFish ManyTheFish added no breaking The related changes are not breaking (DB nor API) bug Something isn't working labels Dec 12, 2022
@Kerollmops Kerollmops changed the title Fix typo initial candiddates computation Fix typo initial candidates computation Dec 13, 2022
irevoire
irevoire previously approved these changes Dec 13, 2022
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

Nice, from what I understand it works.

I looked at all the other criterion and it look like some (proximity, geo, arc_desc, attribute, exactness) are doing this;

                        match initial_candidates {
                            Some(initial_candidates) => {
                                self.initial_candidates |= initial_candidates
                            }
                            None => self.initial_candidates.map_inplace(|c| c | &candidates),
                        }

While words is doing what you wrote here.
Is there a reason to chose one method over the other?

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Looks amazing to me!
bors merge

@ManyTheFish
Copy link
Member Author

ManyTheFish commented Dec 13, 2022

@irevoire, Words and Typo criteria contain an Option<InitialCandidates> instead of InitialCandidates, so I had to make a different match.
This comes from the fact that these two criteria are "lazier" than the others because they may not compute any candidates at their step letting the following criterion do it for them.

@ManyTheFish
Copy link
Member Author

Poke @curquiza about this fix.

@bors
Copy link
Contributor

bors bot commented Dec 13, 2022

@bors bors bot merged commit 406ee31 into main Dec 13, 2022
@bors bors bot deleted the fix-typo-initial-candidates-count branch December 13, 2022 10:49
@curquiza
Copy link
Member

curquiza commented Dec 13, 2022

Is it normal we haven't added any tests here? At least both of the cases reported by the users

bors bot added a commit that referenced this pull request Dec 13, 2022
741: Add test reproducing the bug fixed by #737 r=Kerollmops a=ManyTheFish

related to #737

Co-authored-by: ManyTheFish <many@meilisearch.com>
bors bot added a commit that referenced this pull request Dec 14, 2022
739: Cherry pick bug fixes for v0.37.3 r=curquiza a=curquiza

Integrate #734 and #737 into the `release-v0.37.3` branch to avoid to release on `main`

<s>Wait for meilisearch/meilisearch#3235 investigation without merging this PR. Indeed, if a bug fix on milli's side, we might wait for it before merging this PR/</s> -> no need to wait, not a bug

Co-authored-by: ManyTheFish <many@meilisearch.com>
Co-authored-by: Loïc Lecrenier <loic.lecrenier@me.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working no breaking The related changes are not breaking (DB nor API)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants