From cc7415bb3185918117f5b60562ffc04704d42994 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Lecrenier?= <loic@meilisearch.com>
Date: Tue, 14 Jun 2022 15:28:34 +0200
Subject: [PATCH] Simplify FilterCondition code, made possible by the new NOT
 operator

---
 filter-parser/src/condition.rs   | 25 +++++--------------------
 filter-parser/src/lib.rs         | 24 +++++++++++-------------
 milli/src/search/facet/filter.rs | 19 -------------------
 3 files changed, 16 insertions(+), 52 deletions(-)

diff --git a/filter-parser/src/condition.rs b/filter-parser/src/condition.rs
index cbf73b96a..e967bd074 100644
--- a/filter-parser/src/condition.rs
+++ b/filter-parser/src/condition.rs
@@ -21,30 +21,12 @@ pub enum Condition<'a> {
     Equal(Token<'a>),
     NotEqual(Token<'a>),
     Exists,
-    NotExists,
     LowerThan(Token<'a>),
     LowerThanOrEqual(Token<'a>),
     Between { from: Token<'a>, to: Token<'a> },
 }
 
-impl<'a> Condition<'a> {
-    /// This method can return two operations in case it must express
-    /// an OR operation for the between case (i.e. `TO`).
-    pub fn negate(self) -> (Self, Option<Self>) {
-        match self {
-            GreaterThan(n) => (LowerThanOrEqual(n), None),
-            GreaterThanOrEqual(n) => (LowerThan(n), None),
-            Equal(s) => (NotEqual(s), None),
-            NotEqual(s) => (Equal(s), None),
-            Exists => (NotExists, None),
-            NotExists => (Exists, None),
-            LowerThan(n) => (GreaterThanOrEqual(n), None),
-            LowerThanOrEqual(n) => (GreaterThan(n), None),
-            Between { from, to } => (LowerThan(from), Some(GreaterThan(to))),
-        }
-    }
-}
-/// condition      = value ("=" | "!=" | ">" | ">=" | "<" | "<=") value
+/// condition      = value ("==" | ">" ...) value
 pub fn parse_condition(input: Span) -> IResult<FilterCondition> {
     let operator = alt((tag("<="), tag(">="), tag("!="), tag("<"), tag(">"), tag("=")));
     let (input, (fid, op, value)) = tuple((parse_value, operator, cut(parse_value)))(input)?;
@@ -73,7 +55,10 @@ pub fn parse_not_exists(input: Span) -> IResult<FilterCondition> {
     let (input, key) = parse_value(input)?;
 
     let (input, _) = tuple((tag("NOT"), multispace1, tag("EXISTS")))(input)?;
-    Ok((input, FilterCondition::Condition { fid: key.into(), op: NotExists }))
+    Ok((
+        input,
+        FilterCondition::Not(Box::new(FilterCondition::Condition { fid: key.into(), op: Exists })),
+    ))
 }
 
 /// to             = value value "TO" WS+ value
diff --git a/filter-parser/src/lib.rs b/filter-parser/src/lib.rs
index 49eba1c61..9b33f6d24 100644
--- a/filter-parser/src/lib.rs
+++ b/filter-parser/src/lib.rs
@@ -117,7 +117,6 @@ pub enum FilterCondition<'a> {
     Or(Vec<Self>),
     And(Vec<Self>),
     GeoLowerThan { point: [Token<'a>; 2], radius: Token<'a> },
-    GeoGreaterThan { point: [Token<'a>; 2], radius: Token<'a> },
 }
 
 impl<'a> FilterCondition<'a> {
@@ -144,7 +143,6 @@ impl<'a> FilterCondition<'a> {
                 None
             }
             FilterCondition::GeoLowerThan { point: [point, _], .. } if depth == 0 => Some(point),
-            FilterCondition::GeoGreaterThan { point: [point, _], .. } if depth == 0 => Some(point),
             _ => None,
         }
     }
@@ -443,17 +441,17 @@ pub mod tests {
             ),
             (
                 "NOT subscribers EXISTS",
-                Fc::Condition {
+                Fc::Not(Box::new(Fc::Condition {
                     fid: rtok("NOT ", "subscribers"),
-                    op: Condition::NotExists,
-                },
+                    op: Condition::Exists,
+                })),
             ),
             (
                 "subscribers NOT EXISTS",
-                Fc::Condition {
+                Fc::Not(Box::new(Fc::Condition {
                     fid: rtok("", "subscribers"),
-                    op: Condition::NotExists,
-                },
+                    op: Condition::Exists,
+                })),
             ),
             (
                 "NOT subscribers NOT EXISTS",
@@ -464,10 +462,10 @@ pub mod tests {
             ),
             (
                 "subscribers NOT   EXISTS",
-                Fc::Condition {
+                Fc::Not(Box::new(Fc::Condition {
                     fid: rtok("", "subscribers"),
-                    op: Condition::NotExists,
-                },
+                    op: Condition::Exists,
+                })),
             ),
             (
                 "subscribers 100 TO 1000",
@@ -503,10 +501,10 @@ pub mod tests {
             ),
             (
                 "NOT _geoRadius(12, 13, 14)",
-                Fc::GeoGreaterThan {
+                Fc::Not(Box::new(Fc::GeoLowerThan {
                     point: [rtok("NOT _geoRadius(", "12"), rtok("NOT _geoRadius(12, ", "13")],
                     radius: rtok("NOT _geoRadius(12, 13, ", "14"),
-                },
+                })),
             ),
             // test simple `or` and `and`
             (
diff --git a/milli/src/search/facet/filter.rs b/milli/src/search/facet/filter.rs
index 23917e4aa..ac3215dea 100644
--- a/milli/src/search/facet/filter.rs
+++ b/milli/src/search/facet/filter.rs
@@ -276,14 +276,6 @@ impl<'a> Filter<'a> {
                 let exist = index.exists_faceted_documents_ids(rtxn, field_id)?;
                 return Ok(exist);
             }
-            Condition::NotExists => {
-                let all_ids = index.documents_ids(rtxn)?;
-
-                let exist = Self::evaluate_operator(rtxn, index, field_id, &Condition::Exists)?;
-
-                let notexist = all_ids - exist;
-                return Ok(notexist);
-            }
             Condition::Equal(val) => {
                 let (_original_value, string_docids) = strings_db
                     .get(rtxn, &(field_id, &val.value().to_lowercase()))?
@@ -460,17 +452,6 @@ impl<'a> Filter<'a> {
                     }))?;
                 }
             }
-            FilterCondition::GeoGreaterThan { point, radius } => {
-                let result = Self::inner_evaluate(
-                    &FilterCondition::GeoLowerThan { point: point.clone(), radius: radius.clone() }
-                        .into(),
-                    rtxn,
-                    index,
-                    filterable_fields,
-                )?;
-                let geo_faceted_doc_ids = index.geo_faceted_documents_ids(rtxn)?;
-                Ok(geo_faceted_doc_ids - result)
-            }
         }
     }
 }