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

Fix bug in filter search #727

Merged
merged 5 commits into from
Dec 7, 2022
Merged

Fix bug in filter search #727

merged 5 commits into from
Dec 7, 2022

Conversation

loiclec
Copy link
Contributor

@loiclec loiclec commented Dec 7, 2022

Pull Request

Related issue

Fixes (partially, until merged into meilisearch) meilisearch/meilisearch#3178

What does this PR do?

The most important change is this one:

    // in milli/src/search/facet/facet_range_search.rs, line 239
    let should_stop = {
        match self.right {
            Bound::Included(right) => right < previous_key.left_bound,
            Bound::Excluded(right) => right <= previous_key.left_bound,
            Bound::Unbounded => false,
        }
    };

where the operations < and <= between the two branches were switched. This caused (very few) documents to be missing from filter results.

The second change is a simplification of the algorithm for filters such as field = value, where we now perform a direct query into the "Level 0" of the facet db to retrieve the docids instead of invoking the full facet search algorithm. This change is done in milli/src/search/facet/filter.rs.

I have added yet more insta-snapshot tests, rechecked the content of the snapshots, and added some integration tests as well.

This is purely a fix in the search algorithms. Based on this PR alone, a dump will not be necessary to switch from v0.30.1 (where this bug is present) to v0.30.2 (where this PR is merged).

By creating snapshots and updating the format of the existing
snapshots. The next commit will apply the fix, which will show
its effects cleanly on the old and new snapshot tests
@loiclec loiclec added bug Something isn't working no breaking The related changes are not breaking (DB nor API) labels Dec 7, 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.

pfiou, thank you for sorting that out!

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.

A love what you did!

milli/src/search/facet/filter.rs Show resolved Hide resolved
milli/src/search/facet/facet_range_search.rs Show resolved Hide resolved
milli/src/search/facet/filter.rs Show resolved Hide resolved
@Kerollmops
Copy link
Member

bors merge

@bors
Copy link
Contributor

bors bot commented Dec 7, 2022

@bors bors bot merged commit 098c410 into main Dec 7, 2022
@bors bors bot deleted the facet-search-fix branch December 7, 2022 14:57
@curquiza curquiza mentioned this pull request Dec 8, 2022
bors bot added a commit that referenced this pull request Dec 8, 2022
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.
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.

None yet

3 participants