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

Simplify some unit tests #600

Merged
merged 23 commits into from
Aug 4, 2022
Merged

Simplify some unit tests #600

merged 23 commits into from
Aug 4, 2022

Conversation

loiclec
Copy link
Contributor

@loiclec loiclec commented Aug 3, 2022

Pull Request

What does this PR do?

Simplify the code that is used in unit tests to create and modify an index. Basically, the following code:

  let path = tempfile::tempdir().unwrap();
  let mut options = EnvOpenOptions::new();
  options.map_size(10 * 1024 * 1024); // 10 MB
  let index = Index::new(options, &path).unwrap();

  let mut wtxn = index.write_txn().unwrap();
  let content = documents!([
      { "id": 0, "name": "kevin" },
  ]);
  let config = IndexerConfig::default();
  let indexing_config = IndexDocumentsConfig::default();
  let builder =
      IndexDocuments::new(&mut wtxn, &index, &config, indexing_config, |_| ()).unwrap();
  let (builder, user_error) = builder.add_documents(content).unwrap();
  user_error.unwrap();
  builder.execute().unwrap();
  wtxn.commit.unwrap();

  let mut wtxn = index.write_txn().unwrap();
  let config = IndexerConfig::default();
  let mut builder = Settings::new(&mut wtxn, &index, &config);
  builder.set_primary_key(S("docid"));
  builder.set_filterable_fields(hashset! { S("label") });
  builder.execute(|_| ()).unwrap();
  wtxn.commit().unwrap();

becomes:

let index = TempIndex::new():
index.add_documents(documents!(
      { "id": 0, "name": "kevin" },
)).unwrap();
index.update_settings(|settings| {
    settings.set_primary_key(S("docid"));
    settings.set_filterable_fields(hashset! { S("label") });
}).unwrap();

Then there is a bunch of options to modify the indexing configs, the map size, to reuse a transaction, etc. For example:

let mut index = TempIndex::new_with_map_size(1000 * 4096 * 10);
index.index_documents_config.autogenerate_docids = true;
let mut wtxn = index.write_txn().unwrap();
index.update_settings_using_wtxn(&mut wtxn, |settings| {
    settings.set_primary_key(S("docids"));
}).unwrap();
wtxn.commit().unwrap();

Loïc Lecrenier and others added 20 commits July 19, 2022 10:07
Example:
```json
{
    "id": 0,
    "colour" : { "green": 1 }
}
```
becomes:
```json
{
    "id": 0,
    "colour" : [],
    "colour.green": 1
}
```
to retain the information the key "colour" exists in the original
json value.
Allow multiple spaces between NOT and EXISTS
Co-authored-by: Tamo <tamo@meilisearch.com>
OR, AND, NOT, TO must now be followed by spaces
The idea is to directly create a sorted and merged list of bitmaps
in the form of a BTreeMap<FieldId, RoaringBitmap> instead of creating
a grenad::Reader where the keys are field_id and the values are docids.

Then we send that BTreeMap to the thing that handles TypedChunks, which
inserts its content into the database.
Co-authored-by: Many the fish <many@meilisearch.com>
@loiclec loiclec added the no breaking The related changes are not breaking (DB nor API) label Aug 3, 2022
@loiclec loiclec requested a review from irevoire August 3, 2022 15:02
Copy link
Member

@ManyTheFish ManyTheFish left a comment

Choose a reason for hiding this comment

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

Nice PR -619 lines!

I just requested some formatting changes.

Could you rebase your branch, please? Unfortunately, I added a new test recently that does not follow your new test structure 😬

milli/src/index.rs Show resolved Hide resolved
milli/src/index.rs Show resolved Hide resolved
bors bot and others added 3 commits August 4, 2022 09:46
556: Add EXISTS filter r=loiclec a=loiclec

## What does this PR do?

Fixes issue [#2484](meilisearch/meilisearch#2484) in the meilisearch repo.

It creates a `field EXISTS` filter which selects all documents containing the `field` key. 
For example, with the following documents:
```json
[{
	"id": 0,
	"colour": []
},
{
	"id": 1,
	"colour": ["blue", "green"]
},
{
	"id": 2,
	"colour": 145238
},
{
	"id": 3,
	"colour": null
},
{
	"id": 4,
	"colour": {
		"green": []
	}
},
{
	"id": 5,
	"colour": {}
},
{
	"id": 6
}]
```
Then the filter `colour EXISTS` selects the ids `[0, 1, 2, 3, 4, 5]`. The filter `colour NOT EXISTS` selects `[6]`.

## Details
There is a new database named `facet-id-exists-docids`. Its keys are field ids and its values are bitmaps of all the document ids where the corresponding field exists.

To create this database, the indexing part of milli had to be adapted. The implementation there is basically copy/pasted from the code handling the `facet-id-f64-docids` database, with appropriate modifications in place.

There was an issue involving the flattening of documents during (re)indexing. Previously, the following JSON:
```json
{
    "id": 0,
    "colour": [],
    "size": {}
}
```
would be flattened to:
```json
{
    "id": 0
}
```
prior to being given to the extraction pipeline.

This transformation would lose the information that is needed to populate the `facet-id-exists-docids` database. Therefore, I have also changed the implementation of the `flatten-serde-json` crate. Now, as it traverses the Json, it keeps track of which key was encountered. Then, at the end, if a previously encountered key is not present in the flattened object, it adds that key to the object with an empty array as value. For example:
```json
{
    "id": 0,
    "colour": {
        "green": [],
        "blue": 1
    },
    "size": {}
} 
```
becomes
```json
{
    "id": 0,
    "colour": [],
    "colour.green": [],
    "colour.blue": 1,
    "size": []
} 
```


Co-authored-by: Kerollmops <clement@meilisearch.com>
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.

noice

Copy link
Member

@ManyTheFish ManyTheFish 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 @loiclec,

bors merge

@bors
Copy link
Contributor

bors bot commented Aug 4, 2022

@bors bors bot merged commit 950d8e4 into main Aug 4, 2022
@bors bors bot deleted the simplify-tests branch August 4, 2022 10:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no breaking The related changes are not breaking (DB nor API)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants