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

Fix bulk facet indexing bug #712

Merged
merged 1 commit into from
Nov 30, 2022
Merged

Fix bulk facet indexing bug #712

merged 1 commit into from
Nov 30, 2022

Conversation

loiclec
Copy link
Contributor

@loiclec loiclec commented Nov 30, 2022

Pull Request

Related issue

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

What does this PR do?

Fixes a bug where indexing certain numbers of filterable attribute values in bulk led to corrupted facet databases. This was due to a lossy integer conversion which would ultimately prevent entire levels of the facet database to be written into LMDB.

More specifically, this change was made:

      - if cur_writer_len as u8 >= self.min_level_size {
      + if cur_writer_len >= self.min_level_size as usize {

I also checked other comparisons to min_level_size and other conversions such as x as u8 in this part of the codebase.

@loiclec loiclec added bug Something isn't working no breaking The related changes are not breaking (DB nor API) labels Nov 30, 2022
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.

Thank you very much for this fix 🎉 Can you confirm that a dump will be required to move to the next version of Meilisearch?

bors merge

@bors
Copy link
Contributor

bors bot commented Nov 30, 2022

@bors bors bot merged commit e1612fc into main Nov 30, 2022
@bors bors bot deleted the fix-bulk-facet-indexing-bug branch November 30, 2022 17:14
@loiclec
Copy link
Contributor Author

loiclec commented Dec 1, 2022

Can you confirm that a dump will be required to move to the next version of Meilisearch?

yes, a dump will be required :(

Kerollmops pushed a commit that referenced this pull request Dec 6, 2022
712: Fix bulk facet indexing bug r=Kerollmops a=loiclec

# Pull Request

## Related issue
Fixes (partially, until merged into meilisearch) meilisearch/meilisearch#3165

## What does this PR do?
Fixes a bug where indexing certain numbers of filterable attribute values in bulk led to corrupted facet databases. This was due to a lossy integer conversion which would ultimately prevent entire levels of the facet database to be written into LMDB.

More specifically, this change was made:
```diff
      - if cur_writer_len as u8 >= self.min_level_size {
      + if cur_writer_len >= self.min_level_size as usize {
```
I also checked other comparisons to `min_level_size` and other conversions such as `x as u8` in this part of the codebase.



Co-authored-by: Loïc Lecrenier <loic.lecrenier@me.com>
bors bot added a commit that referenced this pull request Dec 6, 2022
725: Reapply fixes for v0.37.1 r=curquiza a=Kerollmops

This PR reapplies #712, #720, #722, and #723.
We needed the #720 as it was bringing more tests and snap-related improvements.

Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.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.

2 participants