Merge #727

727: Fix bug in filter search r=Kerollmops a=loiclec

# 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:
```rust
    // 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).


Co-authored-by: Loïc Lecrenier <[email protected]>

Author: bors[bot]

milli/src/search/facet/facet_range_search.rs

@@ -171,6 +171,8 @@ impl<'t, 'b, 'bitmap> FacetRangeSearch<'t, 'b, 'bitmap> {
             }
 
             // should we stop?
+            // We should if the the search range doesn't include any
+            // element from the previous key or its successors
             let should_stop = {
                 match self.right {
                     Bound::Included(right) => right < previous_key.left_bound,
@@ -233,10 +235,12 @@ impl<'t, 'b, 'bitmap> FacetRangeSearch<'t, 'b, 'bitmap> {
         }
 
         // should we stop?
+        // We should if the the search range doesn't include any
+        // element from the previous key or its successors
         let should_stop = {
             match self.right {
-                Bound::Included(right) => right <= previous_key.left_bound,
-                Bound::Excluded(right) => right < previous_key.left_bound,
+                Bound::Included(right) => right < previous_key.left_bound,
+                Bound::Excluded(right) => right <= previous_key.left_bound,
                 Bound::Unbounded => false,
             }
         };
@@ -321,8 +325,27 @@ mod tests {
     #[test]
     fn random_looking_index_snap() {
         let index = get_random_looking_index();
-        milli_snap!(format!("{index}"));
+        milli_snap!(format!("{index}"), @"3256c76a7c1b768a013e78d5fa6e9ff9");
     }
+
+    #[test]
+    fn random_looking_index_with_multiple_field_ids_snap() {
+        let index = get_random_looking_index_with_multiple_field_ids();
+        milli_snap!(format!("{index}"), @"c3e5fe06a8f1c404ed4935b32c90a89b");
+    }
+
+    #[test]
+    fn simple_index_snap() {
+        let index = get_simple_index();
+        milli_snap!(format!("{index}"), @"5dbfa134cc44abeb3ab6242fc182e48e");
+    }
+
+    #[test]
+    fn simple_index_with_multiple_field_ids_snap() {
+        let index = get_simple_index_with_multiple_field_ids();
+        milli_snap!(format!("{index}"), @"a4893298218f682bc76357f46777448c");
+    }
+
     #[test]
     fn filter_range_increasing() {
         let indexes = [
@@ -349,7 +372,7 @@ mod tests {
                 )
                 .unwrap();
                 #[allow(clippy::format_push_string)]
-                results.push_str(&format!("{}\n", display_bitmap(&docids)));
+                results.push_str(&format!("0 <= . <= {i} : {}\n", display_bitmap(&docids)));
             }
             milli_snap!(results, format!("included_{i}"));
             let mut results = String::new();
@@ -368,7 +391,7 @@ mod tests {
                 )
                 .unwrap();
                 #[allow(clippy::format_push_string)]
-                results.push_str(&format!("{}\n", display_bitmap(&docids)));
+                results.push_str(&format!("0 < . < {i} : {}\n", display_bitmap(&docids)));
             }
             milli_snap!(results, format!("excluded_{i}"));
             txn.commit().unwrap();
@@ -401,7 +424,7 @@ mod tests {
                     &mut docids,
                 )
                 .unwrap();
-                results.push_str(&format!("{}\n", display_bitmap(&docids)));
+                results.push_str(&format!("{i} <= . <= 255 : {}\n", display_bitmap(&docids)));
             }
 
             milli_snap!(results, format!("included_{i}"));
@@ -422,7 +445,7 @@ mod tests {
                     &mut docids,
                 )
                 .unwrap();
-                results.push_str(&format!("{}\n", display_bitmap(&docids)));
+                results.push_str(&format!("{i} < . < 255 : {}\n", display_bitmap(&docids)));
             }
 
             milli_snap!(results, format!("excluded_{i}"));
@@ -457,7 +480,11 @@ mod tests {
                     &mut docids,
                 )
                 .unwrap();
-                results.push_str(&format!("{}\n", display_bitmap(&docids)));
+                results.push_str(&format!(
+                    "{i} <= . <= {r} : {docids}\n",
+                    r = 255. - i,
+                    docids = display_bitmap(&docids)
+                ));
             }
 
             milli_snap!(results, format!("included_{i}"));
@@ -478,12 +505,148 @@ mod tests {
                     &mut docids,
                 )
                 .unwrap();
-                results.push_str(&format!("{}\n", display_bitmap(&docids)));
+                results.push_str(&format!(
+                    "{i} <  . < {r} {docids}\n",
+                    r = 255. - i,
+                    docids = display_bitmap(&docids)
+                ));
             }
 
             milli_snap!(results, format!("excluded_{i}"));
 
             txn.commit().unwrap();
         }
     }
+
+    #[test]
+    fn filter_range_unbounded() {
+        let indexes = [
+            get_simple_index(),
+            get_random_looking_index(),
+            get_simple_index_with_multiple_field_ids(),
+            get_random_looking_index_with_multiple_field_ids(),
+        ];
+        for (i, index) in indexes.iter().enumerate() {
+            let txn = index.env.read_txn().unwrap();
+            let mut results = String::new();
+            for i in 0..=255 {
+                let i = i as f64;
+                let start = Bound::Included(i);
+                let end = Bound::Unbounded;
+                let mut docids = RoaringBitmap::new();
+                find_docids_of_facet_within_bounds::<OrderedF64Codec>(
+                    &txn,
+                    index.content.remap_key_type::<FacetGroupKeyCodec<OrderedF64Codec>>(),
+                    0,
+                    &start,
+                    &end,
+                    &mut docids,
+                )
+                .unwrap();
+                #[allow(clippy::format_push_string)]
+                results.push_str(&format!(">= {i}: {}\n", display_bitmap(&docids)));
+            }
+            milli_snap!(results, format!("start_from_included_{i}"));
+            let mut results = String::new();
+            for i in 0..=255 {
+                let i = i as f64;
+                let start = Bound::Unbounded;
+                let end = Bound::Included(i);
+                let mut docids = RoaringBitmap::new();
+                find_docids_of_facet_within_bounds::<OrderedF64Codec>(
+                    &txn,
+                    index.content.remap_key_type::<FacetGroupKeyCodec<OrderedF64Codec>>(),
+                    0,
+                    &start,
+                    &end,
+                    &mut docids,
+                )
+                .unwrap();
+                #[allow(clippy::format_push_string)]
+                results.push_str(&format!("<= {i}: {}\n", display_bitmap(&docids)));
+            }
+            milli_snap!(results, format!("end_at_included_{i}"));
+
+            let mut docids = RoaringBitmap::new();
+            find_docids_of_facet_within_bounds::<OrderedF64Codec>(
+                &txn,
+                index.content.remap_key_type::<FacetGroupKeyCodec<OrderedF64Codec>>(),
+                0,
+                &Bound::Unbounded,
+                &Bound::Unbounded,
+                &mut docids,
+            )
+            .unwrap();
+            milli_snap!(
+                &format!("all field_id 0: {}\n", display_bitmap(&docids)),
+                format!("unbounded_field_id_0_{i}")
+            );
+
+            let mut docids = RoaringBitmap::new();
+            find_docids_of_facet_within_bounds::<OrderedF64Codec>(
+                &txn,
+                index.content.remap_key_type::<FacetGroupKeyCodec<OrderedF64Codec>>(),
+                1,
+                &Bound::Unbounded,
+                &Bound::Unbounded,
+                &mut docids,
+            )
+            .unwrap();
+            milli_snap!(
+                &format!("all field_id 1:  {}\n", display_bitmap(&docids)),
+                format!("unbounded_field_id_1_{i}")
+            );
+
+            drop(txn);
+        }
+    }
+
+    #[test]
+    fn filter_range_exact() {
+        let indexes = [
+            get_simple_index(),
+            get_random_looking_index(),
+            get_simple_index_with_multiple_field_ids(),
+            get_random_looking_index_with_multiple_field_ids(),
+        ];
+        for (i, index) in indexes.iter().enumerate() {
+            let txn = index.env.read_txn().unwrap();
+            let mut results_0 = String::new();
+            let mut results_1 = String::new();
+            for i in 0..=255 {
+                let i = i as f64;
+                let start = Bound::Included(i);
+                let end = Bound::Included(i);
+                let mut docids = RoaringBitmap::new();
+                find_docids_of_facet_within_bounds::<OrderedF64Codec>(
+                    &txn,
+                    index.content.remap_key_type::<FacetGroupKeyCodec<OrderedF64Codec>>(),
+                    0,
+                    &start,
+                    &end,
+                    &mut docids,
+                )
+                .unwrap();
+                #[allow(clippy::format_push_string)]
+                results_0.push_str(&format!("{i}: {}\n", display_bitmap(&docids)));
+
+                let mut docids = RoaringBitmap::new();
+                find_docids_of_facet_within_bounds::<OrderedF64Codec>(
+                    &txn,
+                    index.content.remap_key_type::<FacetGroupKeyCodec<OrderedF64Codec>>(),
+                    1,
+                    &start,
+                    &end,
+                    &mut docids,
+                )
+                .unwrap();
+                #[allow(clippy::format_push_string)]
+                results_1.push_str(&format!("{i}: {}\n", display_bitmap(&docids)));
+            }
+            milli_snap!(results_0, format!("field_id_0_exact_{i}"));
+            milli_snap!(results_1, format!("field_id_1_exact_{i}"));
+
+            drop(txn);
+        }
+    }
 }