tweak error handling

milli/src/error.rs

@@ -210,6 +210,8 @@ impl fmt::Display for UserError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
             //TODO
+            Self::InvalidFilterAttributeNom => write!(f, "parser error "),
+            Self::InvalidFilterValue => write!(f, "parser error "),
             Self::InvalidFilterNom { input } => write!(f, "parser error {}", input),
             Self::AttributeLimitReached => f.write_str("maximum number of attributes reached"),
             Self::CriterionError(error) => write!(f, "{}", error),
@@ -228,10 +230,6 @@ impl fmt::Display for UserError {
                 "the document with the id: {} contains an invalid _geo field: {}",
                 document_id, object
             ),
-            Self::InvalidAscDescSyntax { name } => {
-                write!(f, "invalid asc/desc syntax for {}", name)
-            }
-            Self::InvalidCriterionName { name } => write!(f, "invalid criterion {}", name),
             Self::InvalidDocumentId { document_id } => {
                 let json = serde_json::to_string(document_id).unwrap();
                 write!(

milli/src/search/facet/filter_condition.rs

@@ -5,7 +5,7 @@ use std::ops::Bound::{self, Excluded, Included};
 use either::Either;
 use heed::types::DecodeIgnore;
 use log::debug;
-use nom::error::VerboseError;
+use nom::error::{convert_error, VerboseError};
 use roaring::RoaringBitmap;
 
 use self::FilterCondition::*;
@@ -87,7 +87,15 @@ impl FilterCondition {
             ParseContext { fields_ids_map: &fields_ids_map, filterable_fields: &filterable_fields };
         match ctx.parse_expression::<VerboseError<&str>>(expression) {
             Ok((_, fc)) => Ok(fc),
-            Err(e) => Err(Error::UserError(UserError::InvalidFilterNom { input: e.to_string() })),
+            Err(e) => {
+                match e {
+                    nom::Err::Error(x) => {
+                        println!("verbose err:\n{}", convert_error(expression, x))
+                    }
+                    _ => unreachable!(),
+                }
+                Err(Error::UserError(UserError::InvalidFilterNom { input: "whatever".to_string() }))
+            }
         }
     }
     pub fn negate(self) -> FilterCondition {
@@ -101,67 +109,6 @@ impl FilterCondition {
             Empty => Empty,
         }
     }
-
-    fn geo_radius(
-        fields_ids_map: &FieldsIdsMap,
-        filterable_fields: &HashSet<String>,
-        item: Pair<Rule>,
-    ) -> Result<FilterCondition> {
-        if !filterable_fields.contains("_geo") {
-            return Err(UserError::InvalidFilterAttribute(PestError::new_from_span(
-                ErrorVariant::CustomError {
-                    message: format!(
-                    "attribute `_geo` is not filterable, available filterable attributes are: {}",
-                    filterable_fields.iter().join(", "),
-                ),
-                },
-                item.as_span(),
-            )))?;
-        }
-        let mut items = item.into_inner();
-        let fid = match fields_ids_map.id("_geo") {
-            Some(fid) => fid,
-            None => return Ok(Empty),
-        };
-        let parameters_item = items.next().unwrap();
-        // We don't need more than 3 parameters, but to handle errors correctly we are still going
-        // to extract the first 4 parameters
-        let param_span = parameters_item.as_span();
-        let parameters = parameters_item
-            .into_inner()
-            .take(4)
-            .map(|param| (param.clone(), param.as_span()))
-            .map(|(param, span)| pest_parse(param).0.map(|arg| (arg, span)))
-            .collect::<StdResult<Vec<(f64, _)>, _>>()
-            .map_err(UserError::InvalidFilter)?;
-        if parameters.len() != 3 {
-            return Err(UserError::InvalidFilter(PestError::new_from_span(
-                        ErrorVariant::CustomError {
-                            message: format!("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`"),
-                        },
-                        // we want to point to the last parameters and if there was no parameters we
-                        // point to the parenthesis
-                        parameters.last().map(|param| param.1.clone()).unwrap_or(param_span),
-            )))?;
-        }
-        let (lat, lng, distance) = (&parameters[0], &parameters[1], parameters[2].0);
-        if !(-90.0..=90.0).contains(&lat.0) {
-            return Err(UserError::InvalidFilter(PestError::new_from_span(
-                ErrorVariant::CustomError {
-                    message: format!("Latitude must be contained between -90 and 90 degrees."),
-                },
-                lat.1.clone(),
-            )))?;
-        } else if !(-180.0..=180.0).contains(&lng.0) {
-            return Err(UserError::InvalidFilter(PestError::new_from_span(
-                ErrorVariant::CustomError {
-                    message: format!("Longitude must be contained between -180 and 180 degrees."),
-                },
-                lng.1.clone(),
-            )))?;
-        }
-        Ok(Operator(fid, Operator::GeoLowerThan([lat.0, lng.0], distance)))
-    }
 }
 
 impl FilterCondition {
@@ -348,7 +295,7 @@ impl FilterCondition {
                     numbers_db,
                     strings_db,
                     field_id,
-                    &GeoLowerThan(point.clone(), *distance),
+                    &Operator::GeoLowerThan(point.clone(), *distance),
                 )?;
                 let geo_faceted_doc_ids = index.geo_faceted_documents_ids(rtxn)?;
                 return Ok(geo_faceted_doc_ids - result);

milli/src/search/facet/filter_parser.rs

@@ -9,10 +9,11 @@ use nom::{
     bytes::complete::{tag, take_while1},
     character::complete::{char, multispace0},
     combinator::map,
-    error::ErrorKind,
     error::ParseError,
     error::VerboseError,
+    error::{ContextError, ErrorKind},
     multi::many0,
+    multi::separated_list1,
     sequence::{delimited, preceded, tuple},
     IResult,
 };
@@ -43,6 +44,8 @@ impl Operator {
             LowerThan(n) => (GreaterThanOrEqual(n), None),
             LowerThanOrEqual(n) => (GreaterThan(n), None),
             Between(n, m) => (LowerThan(n), Some(GreaterThan(m))),
+            GeoLowerThan(point, distance) => (GeoGreaterThan(point, distance), None),
+            GeoGreaterThan(point, distance) => (GeoLowerThan(point, distance), None),
         }
     }
 }
@@ -193,13 +196,52 @@ impl<'a> ParseContext<'a> {
         Ok((input, res))
     }
 
+    fn parse_geo_radius<E>(&'a self, input: &'a str) -> IResult<&'a str, FilterCondition, E>
+    where
+        E: ParseError<&'a str>,
+    {
+        let (input, args) = preceded(
+            tag("_geoRadius"),
+            delimited(
+                tag("("),
+                separated_list1(tag(","), self.ws(|c| self.parse_value(c))),
+                tag(")"),
+            ),
+        )(input)?;
+
+        if args.len() != 3 {
+            let e = E::from_char(input, '(');
+            return Err(nom::Err::Failure(e));
+        }
+        let lat = self.parse_numeric(args[0])?;
+        let lng = self.parse_numeric(args[1])?;
+        let dis = self.parse_numeric(args[2])?;
+
+        let fid = match self.fields_ids_map.id("_geo") {
+            Some(fid) => fid,
+            None => return Ok((input, FilterCondition::Empty)),
+        };
+
+        if let Some(span) = (!(-181.0..181.).contains(&lat))
+            .then(|| &lat)
+            .or((!(-181.0..181.).contains(&lng)).then(|| &lng))
+        {
+            let e = E::from_char(input, '(');
+            return Err(nom::Err::Failure(e));
+        }
+
+        let res = FilterCondition::Operator(fid, GeoLowerThan([lat, lng], dis));
+        Ok((input, res))
+    }
+
     fn parse_condition<E>(&'a self, input: &'a str) -> IResult<&'a str, FilterCondition, E>
     where
         E: ParseError<&'a str>,
     {
+        let l0 = |c| self.parse_geo_radius(c);
         let l1 = |c| self.parse_simple_condition(c);
         let l2 = |c| self.parse_range_condition(c);
-        let (input, condition) = alt((l1, l2))(input)?;
+        let (input, condition) = alt((l0, l1, l2))(input)?;
         Ok((input, condition))
     }
 
@@ -220,6 +262,15 @@ impl<'a> ParseContext<'a> {
         let key = |input| take_while1(Self::is_key_component)(input);
         alt((key, delimited(char('"'), key, char('"'))))(input)
     }
+
+    fn parse_value<E>(&'a self, input: &'a str) -> IResult<&'a str, &'a str, E>
+    where
+        E: ParseError<&'a str>,
+    {
+        let key = |input| take_while1(Self::is_key_component)(input);
+        alt((key, delimited(char('"'), key, char('"'))))(input)
+    }
+
     fn is_key_component(c: char) -> bool {
         c.is_alphanumeric() || ['_', '-', '.'].contains(&c)
     }
@@ -431,13 +482,13 @@ mod tests {
         // basic test
         let condition =
             FilterCondition::from_str(&rtxn, &index, "_geoRadius(12, 13.0005, 2000)").unwrap();
-        let expected = Operator(0, GeoLowerThan([12., 13.0005], 2000.));
+        let expected = FilterCondition::Operator(0, GeoLowerThan([12., 13.0005], 2000.));
         assert_eq!(condition, expected);
 
         // test the negation of the GeoLowerThan
         let condition =
             FilterCondition::from_str(&rtxn, &index, "NOT _geoRadius(50, 18, 2000.500)").unwrap();
-        let expected = Operator(0, GeoGreaterThan([50., 18.], 2000.500));
+        let expected = FilterCondition::Operator(0, GeoGreaterThan([50., 18.], 2000.500));
         assert_eq!(condition, expected);
 
         // composition of multiple operations
@@ -446,13 +497,13 @@ mod tests {
             &index,
             "(NOT _geoRadius(1, 2, 300) AND _geoRadius(1.001, 2.002, 1000.300)) OR price <= 10",
         )
-        .unwrap();
+        .unwrap_or_else(|e| FilterCondition::Empty);
-        let expected = Or(
+        let expected = FilterCondition::Or(
-            Box::new(And(
+            Box::new(FilterCondition::And(
-                Box::new(Operator(0, GeoGreaterThan([1., 2.], 300.))),
+                Box::new(FilterCondition::Operator(0, GeoGreaterThan([1., 2.], 300.))),
-                Box::new(Operator(0, GeoLowerThan([1.001, 2.002], 1000.300))),
+                Box::new(FilterCondition::Operator(0, GeoLowerThan([1.001, 2.002], 1000.300))),
             )),
-            Box::new(Operator(1, LowerThanOrEqual(10.))),
+            Box::new(FilterCondition::Operator(1, LowerThanOrEqual(10.))),
         );
         assert_eq!(condition, expected);