diff --git a/milli/src/search/facet/filter_condition.rs b/milli/src/search/facet/filter_condition.rs index e6ed79230..d51ae27cd 100644 --- a/milli/src/search/facet/filter_condition.rs +++ b/milli/src/search/facet/filter_condition.rs @@ -2,15 +2,10 @@ use std::collections::HashSet; use std::fmt::Debug; use std::ops::Bound::{self, Excluded, Included}; use std::result::Result as StdResult; -use std::str::FromStr; use either::Either; use heed::types::DecodeIgnore; -use itertools::Itertools; use log::debug; -use pest::error::{Error as PestError, ErrorVariant}; -use pest::iterators::{Pair, Pairs}; -use pest::Parser; use roaring::RoaringBitmap; use nom::{ @@ -28,7 +23,8 @@ use nom::{ use self::FilterCondition::*; use self::Operator::*; -use super::parser::{FilterParser, Rule, PREC_CLIMBER}; +use super::parser::FilterParser; +use super::parser::{Rule, PREC_CLIMBER}; use super::FacetNumberRange; use crate::error::{Error, UserError}; use crate::heed_codec::facet::{ @@ -145,9 +141,7 @@ impl<'a> ParseContext<'a> { let k = match op { "=" => Operator(fid, Equal(r.ok(), value.to_string().to_lowercase())), "!=" => Operator(fid, NotEqual(r.ok(), value.to_string().to_lowercase())), - ">" | "<" | "<=" | ">=" => { - return self.parse_numeric_unary_condition(input, fid, value) - } + ">" | "<" | "<=" | ">=" => return self.parse_numeric_unary_condition(op, fid, value), _ => unreachable!(), }; Ok((input, k)) @@ -326,92 +320,6 @@ impl FilterCondition { } impl FilterCondition { - pub fn from_array_pest( - rtxn: &heed::RoTxn, - index: &Index, - array: I, - ) -> Result> - where - I: IntoIterator>, - J: IntoIterator, - A: AsRef, - B: AsRef, - { - let mut ands = None; - - for either in array { - match either { - Either::Left(array) => { - let mut ors = None; - for rule in array { - let condition = FilterCondition::from_str(rtxn, index, rule.as_ref())?; - ors = match ors.take() { - Some(ors) => Some(Or(Box::new(ors), Box::new(condition))), - None => Some(condition), - }; - } - - if let Some(rule) = ors { - ands = match ands.take() { - Some(ands) => Some(And(Box::new(ands), Box::new(rule))), - None => Some(rule), - }; - } - } - Either::Right(rule) => { - let condition = FilterCondition::from_str(rtxn, index, rule.as_ref())?; - ands = match ands.take() { - Some(ands) => Some(And(Box::new(ands), Box::new(condition))), - None => Some(condition), - }; - } - } - } - - Ok(ands) - } - - pub fn from_str_pest( - rtxn: &heed::RoTxn, - index: &Index, - expression: &str, - ) -> Result { - let fields_ids_map = index.fields_ids_map(rtxn)?; - let filterable_fields = index.filterable_fields(rtxn)?; - let lexed = - FilterParser::parse(Rule::prgm, expression).map_err(UserError::InvalidFilter)?; - FilterCondition::from_pairs(&fields_ids_map, &filterable_fields, lexed) - } - - fn from_pairs( - fim: &FieldsIdsMap, - ff: &HashSet, - expression: Pairs, - ) -> Result { - PREC_CLIMBER.climb( - expression, - |pair: Pair| match pair.as_rule() { - Rule::greater => Ok(Self::greater_than(fim, ff, pair)?), - Rule::geq => Ok(Self::greater_than_or_equal(fim, ff, pair)?), - Rule::eq => Ok(Self::equal(fim, ff, pair)?), - Rule::neq => Ok(Self::equal(fim, ff, pair)?.negate()), - Rule::leq => Ok(Self::lower_than_or_equal(fim, ff, pair)?), - Rule::less => Ok(Self::lower_than(fim, ff, pair)?), - Rule::between => Ok(Self::between(fim, ff, pair)?), - Rule::geo_radius => Ok(Self::geo_radius(fim, ff, pair)?), - Rule::not => Ok(Self::from_pairs(fim, ff, pair.into_inner())?.negate()), - Rule::prgm => Self::from_pairs(fim, ff, pair.into_inner()), - Rule::term => Self::from_pairs(fim, ff, pair.into_inner()), - _ => unreachable!(), - }, - |lhs: Result, op: Pair, rhs: Result| match op.as_rule() { - Rule::or => Ok(Or(Box::new(lhs?), Box::new(rhs?))), - Rule::and => Ok(And(Box::new(lhs?), Box::new(rhs?))), - _ => unreachable!(), - }, - ) - } - fn negate(self) -> FilterCondition { match self { Operator(fid, op) => match op.negate() { @@ -484,128 +392,6 @@ impl FilterCondition { } Ok(Operator(fid, GeoLowerThan([lat.0, lng.0], distance))) } - - fn between( - fields_ids_map: &FieldsIdsMap, - filterable_fields: &HashSet, - item: Pair, - ) -> Result { - let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { - Some(fid) => fid, - None => return Ok(Empty), - }; - - let (lresult, _) = pest_parse(items.next().unwrap()); - let (rresult, _) = pest_parse(items.next().unwrap()); - - let lvalue = lresult.map_err(UserError::InvalidFilter)?; - let rvalue = rresult.map_err(UserError::InvalidFilter)?; - - Ok(Operator(fid, Between(lvalue, rvalue))) - } - - fn equal( - fields_ids_map: &FieldsIdsMap, - filterable_fields: &HashSet, - item: Pair, - ) -> Result { - let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { - Some(fid) => fid, - None => return Ok(Empty), - }; - - let value = items.next().unwrap(); - let (result, svalue) = pest_parse(value); - - let svalue = svalue.to_lowercase(); - Ok(Operator(fid, Equal(result.ok(), svalue))) - } - - fn greater_than( - fields_ids_map: &FieldsIdsMap, - filterable_fields: &HashSet, - item: Pair, - ) -> Result { - let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { - Some(fid) => fid, - None => return Ok(Empty), - }; - - let value = items.next().unwrap(); - let (result, _svalue) = pest_parse(value); - let value = result.map_err(UserError::InvalidFilter)?; - - Ok(Operator(fid, GreaterThan(value))) - } - - fn greater_than_or_equal( - fields_ids_map: &FieldsIdsMap, - filterable_fields: &HashSet, - item: Pair, - ) -> Result { - let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { - Some(fid) => fid, - None => return Ok(Empty), - }; - - let value = items.next().unwrap(); - let (result, _svalue) = pest_parse(value); - let value = result.map_err(UserError::InvalidFilter)?; - - Ok(Operator(fid, GreaterThanOrEqual(value))) - } - - fn lower_than( - fields_ids_map: &FieldsIdsMap, - filterable_fields: &HashSet, - item: Pair, - ) -> Result { - let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { - Some(fid) => fid, - None => return Ok(Empty), - }; - - let value = items.next().unwrap(); - let (result, _svalue) = pest_parse(value); - let value = result.map_err(UserError::InvalidFilter)?; - - Ok(Operator(fid, LowerThan(value))) - } - - fn lower_than_or_equal( - fields_ids_map: &FieldsIdsMap, - filterable_fields: &HashSet, - item: Pair, - ) -> Result { - let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { - Some(fid) => fid, - None => return Ok(Empty), - }; - - let value = items.next().unwrap(); - let (result, _svalue) = pest_parse(value); - let value = result.map_err(UserError::InvalidFilter)?; - - Ok(Operator(fid, LowerThanOrEqual(value))) - } } impl FilterCondition { @@ -855,72 +641,6 @@ impl FilterCondition { /// /// The pest pair is simply a string associated with a span, a location to highlight in /// the error message. -fn field_id( - fields_ids_map: &FieldsIdsMap, - filterable_fields: &HashSet, - items: &mut Pairs, -) -> StdResult, PestError> { - // lexing ensures that we at least have a key - let key = items.next().unwrap(); - if key.as_rule() == Rule::reserved { - let message = match key.as_str() { - key if key.starts_with("_geoPoint") => { - format!( - "`_geoPoint` is a reserved keyword and thus can't be used as a filter expression. \ - Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates.", - ) - } - key @ "_geo" => { - format!( - "`{}` is a reserved keyword and thus can't be used as a filter expression. \ - Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates.", - key - ) - } - key => format!( - "`{}` is a reserved keyword and thus can't be used as a filter expression.", - key - ), - }; - return Err(PestError::new_from_span(ErrorVariant::CustomError { message }, key.as_span())); - } - - if !filterable_fields.contains(key.as_str()) { - return Err(PestError::new_from_span( - ErrorVariant::CustomError { - message: format!( - "attribute `{}` is not filterable, available filterable attributes are: {}.", - key.as_str(), - filterable_fields.iter().join(", "), - ), - }, - key.as_span(), - )); - } - - Ok(fields_ids_map.id(key.as_str())) -} - -/// Tries to parse the pest pair into the type `T` specified, always returns -/// the original string that we tried to parse. -/// -/// Returns the parsing error associated with the span if the conversion fails. -fn pest_parse(pair: Pair) -> (StdResult>, String) -where - T: FromStr, - T::Err: ToString, -{ - let result = match pair.as_str().parse::() { - Ok(value) => Ok(value), - Err(e) => Err(PestError::::new_from_span( - ErrorVariant::CustomError { message: e.to_string() }, - pair.as_span(), - )), - }; - - (result, pair.as_str().to_string()) -} - #[cfg(test)] mod tests { use big_s::S; @@ -991,6 +711,27 @@ mod tests { assert_eq!(condition, expected); } + #[test] + fn compare() { + 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 mut builder = Settings::new(&mut wtxn, &index, 0); + builder.set_searchable_fields(vec![S("channel"), S("timestamp")]); // to keep the fields order + builder.set_filterable_fields(hashset! { S("channel"), S("timestamp") }); + builder.execute(|_, _| ()).unwrap(); + wtxn.commit().unwrap(); + + let rtxn = index.read_txn().unwrap(); + let condition = FilterCondition::from_str(&rtxn, &index, "channel < 20").unwrap(); + let expected = Operator(0, LowerThan(20.0)); + + assert_eq!(condition, expected); + } + #[test] fn parentheses() { let path = tempfile::tempdir().unwrap();