diff --git a/crates/filter-parser/src/error.rs b/crates/filter-parser/src/error.rs index 9f14ef2e8..88bf90be2 100644 --- a/crates/filter-parser/src/error.rs +++ b/crates/filter-parser/src/error.rs @@ -75,6 +75,7 @@ pub enum ExpectedValueKind { pub enum ErrorKind<'a> { ReservedGeo(&'a str), GeoRadius, + GeoRadiusArgumentCount(usize), GeoBoundingBox, GeoPolygon, GeoPolygonTooFewPoints, @@ -201,7 +202,10 @@ impl Display for Error<'_> { writeln!(f, "Found unexpected characters at the end of the filter: `{}`. You probably forgot an `OR` or an `AND` rule.", escaped_input)? } ErrorKind::GeoRadius => { - writeln!(f, "The `_geoRadius` filter expects three arguments: `_geoRadius(latitude, longitude, radius)`.")? + writeln!(f, "The `_geoRadius` filter must be in the form: `_geoRadius(latitude, longitude, radius, optionalResolution)`.")? + } + ErrorKind::GeoRadiusArgumentCount(count) => { + writeln!(f, "Was expecting 3 or 4 arguments for `_geoRadius`, but instead found {count}.")? } ErrorKind::GeoBoundingBox => { writeln!(f, "The `_geoBoundingBox` filter expects two pairs of arguments: `_geoBoundingBox([latitude, longitude], [latitude, longitude])`.")? diff --git a/crates/filter-parser/src/lib.rs b/crates/filter-parser/src/lib.rs index 458547cc3..c2cc8b991 100644 --- a/crates/filter-parser/src/lib.rs +++ b/crates/filter-parser/src/lib.rs @@ -157,7 +157,7 @@ pub enum FilterCondition<'a> { Or(Vec), And(Vec), VectorExists { fid: Token<'a>, embedder: Option>, filter: VectorFilter<'a> }, - GeoLowerThan { point: [Token<'a>; 2], radius: Token<'a> }, + GeoLowerThan { point: [Token<'a>; 2], radius: Token<'a>, resolution: Option> }, GeoBoundingBox { top_right_point: [Token<'a>; 2], bottom_left_point: [Token<'a>; 2] }, GeoPolygon { points: Vec<[Token<'a>; 2]> }, } @@ -399,23 +399,27 @@ fn parse_not(input: Span, depth: usize) -> IResult { /// If we parse `_geoRadius` we MUST parse the rest of the expression. fn parse_geo_radius(input: Span) -> IResult { // we want to allow space BEFORE the _geoRadius but not after - let parsed = preceded( - tuple((multispace0, word_exact("_geoRadius"))), - // if we were able to parse `_geoRadius` and can't parse the rest of the input we return a failure - cut(delimited(char('('), separated_list1(tag(","), ws(recognize_float)), char(')'))), - )(input) - .map_err(|e| e.map(|_| Error::new_from_kind(input, ErrorKind::GeoRadius))); + + let (input, _) = tuple((multispace0, word_exact("_geoRadius")))(input)?; + + // if we were able to parse `_geoRadius` and can't parse the rest of the input we return a failure + + let parsed = + delimited(char('('), separated_list1(tag(","), ws(recognize_float)), char(')'))(input) + .map_cut(ErrorKind::GeoRadius); let (input, args) = parsed?; - if args.len() != 3 { - return Err(nom::Err::Failure(Error::new_from_kind(input, ErrorKind::GeoRadius))); + if !(3..=4).contains(&args.len()) { + return Err(Error::failure_from_kind(input, ErrorKind::GeoRadiusArgumentCount(args.len()))); } let res = FilterCondition::GeoLowerThan { point: [args[0].into(), args[1].into()], radius: args[2].into(), + resolution: args.get(3).cloned().map(Token::from), }; + Ok((input, res)) } @@ -645,9 +649,12 @@ impl std::fmt::Display for FilterCondition<'_> { } write!(f, " EXISTS") } - FilterCondition::GeoLowerThan { point, radius } => { + FilterCondition::GeoLowerThan { point, radius, resolution: None } => { write!(f, "_geoRadius({}, {}, {})", point[0], point[1], radius) } + FilterCondition::GeoLowerThan { point, radius, resolution: Some(resolution) } => { + write!(f, "_geoRadius({}, {}, {}, {})", point[0], point[1], radius, resolution) + } FilterCondition::GeoBoundingBox { top_right_point: top_left_point, bottom_left_point: bottom_right_point, @@ -831,6 +838,7 @@ pub mod tests { insta::assert_snapshot!(p("_geoRadius(12, 13, 14)"), @"_geoRadius({12}, {13}, {14})"); insta::assert_snapshot!(p("NOT _geoRadius(12, 13, 14)"), @"NOT (_geoRadius({12}, {13}, {14}))"); insta::assert_snapshot!(p("_geoRadius(12,13,14)"), @"_geoRadius({12}, {13}, {14})"); + insta::assert_snapshot!(p("_geoRadius(12,13,14,1000)"), @"_geoRadius({12}, {13}, {14}, {1000})"); // Test geo bounding box insta::assert_snapshot!(p("_geoBoundingBox([12, 13], [14, 15])"), @"_geoBoundingBox([{12}, {13}], [{14}, {15}])"); @@ -919,15 +927,15 @@ pub mod tests { 19:19 channel = Ponce OR "); - insta::assert_snapshot!(p("_geoRadius"), @r###" - The `_geoRadius` filter expects three arguments: `_geoRadius(latitude, longitude, radius)`. - 1:11 _geoRadius - "###); + insta::assert_snapshot!(p("_geoRadius"), @r" + The `_geoRadius` filter must be in the form: `_geoRadius(latitude, longitude, radius, optionalResolution)`. + 11:11 _geoRadius + "); - insta::assert_snapshot!(p("_geoRadius = 12"), @r###" - The `_geoRadius` filter expects three arguments: `_geoRadius(latitude, longitude, radius)`. - 1:16 _geoRadius = 12 - "###); + insta::assert_snapshot!(p("_geoRadius = 12"), @r" + The `_geoRadius` filter must be in the form: `_geoRadius(latitude, longitude, radius, optionalResolution)`. + 11:16 _geoRadius = 12 + "); insta::assert_snapshot!(p("_geoBoundingBox"), @r" The `_geoBoundingBox` filter expects two pairs of arguments: `_geoBoundingBox([latitude, longitude], [latitude, longitude])`. diff --git a/crates/meilisearch/tests/documents/geojson/mod.rs b/crates/meilisearch/tests/documents/geojson/mod.rs index d72873bab..91b5994b1 100644 --- a/crates/meilisearch/tests/documents/geojson/mod.rs +++ b/crates/meilisearch/tests/documents/geojson/mod.rs @@ -382,3 +382,40 @@ async fn geo_bounding_box() { } "#); } + +#[actix_rt::test] +async fn geo_radius() { + let index = shared_index_geojson_documents().await; + + // 200km around Luxembourg + let (response, code) = index + .search_get("?filter=_geoRadius(49.4369862,6.5576591,200000)&attributesToRetrieve=name") + .await; + snapshot!(code, @"200 OK"); + snapshot!(response, @r#" + { + "hits": [ + { + "name": "Belgium" + }, + { + "name": "Germany" + }, + { + "name": "France" + }, + { + "name": "Luxembourg" + }, + { + "name": "Netherlands" + } + ], + "query": "", + "processingTimeMs": "[duration]", + "limit": 20, + "offset": 0, + "estimatedTotalHits": 5 + } + "#); +} diff --git a/crates/milli/src/search/facet/filter.rs b/crates/milli/src/search/facet/filter.rs index 2f1d02982..4904004ce 100644 --- a/crates/milli/src/search/facet/filter.rs +++ b/crates/milli/src/search/facet/filter.rs @@ -38,6 +38,7 @@ pub struct Filter<'a> { pub enum BadGeoError { Lat(f64), Lng(f64), + InvalidResolution(usize), BoundingBoxTopIsBelowBottom(f64, f64), } @@ -49,6 +50,10 @@ impl Display for BadGeoError { Self::BoundingBoxTopIsBelowBottom(top, bottom) => { write!(f, "The top latitude `{top}` is below the bottom latitude `{bottom}`.") } + Self::InvalidResolution(resolution) => write!( + f, + "Invalid resolution `{resolution}`. Resolution must be between 3 and 1000." + ), Self::Lat(lat) => write!( f, "Bad latitude `{}`. Latitude must be contained between -90 and 90 degrees.", @@ -650,17 +655,7 @@ impl<'a> Filter<'a> { FilterCondition::VectorExists { fid: _, embedder, filter } => { super::filter_vector::evaluate(rtxn, index, universe, embedder.clone(), filter) } - FilterCondition::GeoLowerThan { point, radius } => { - if !index.is_geo_filtering_enabled(rtxn)? { - return Err(point[0].as_external_error(FilterError::AttributeNotFilterable { - attribute: RESERVED_GEO_FIELD_NAME, - filterable_patterns: filtered_matching_patterns( - filterable_attribute_rules, - &|features| features.is_filterable(), - ), - }))?; - } - + FilterCondition::GeoLowerThan { point, radius, resolution: res_token } => { let base_point: [f64; 2] = [point[0].parse_finite_float()?, point[1].parse_finite_float()?]; if !(-90.0..=90.0).contains(&base_point[0]) { @@ -670,23 +665,64 @@ impl<'a> Filter<'a> { return Err(point[1].as_external_error(BadGeoError::Lng(base_point[1])))?; } let radius = radius.parse_finite_float()?; - let rtree = match index.geo_rtree(rtxn)? { - Some(rtree) => rtree, - None => return Ok(RoaringBitmap::new()), - }; + let mut resolution = 125; + if let Some(res_token) = res_token { + resolution = res_token.parse_finite_float()? as usize; + if !(3..=1000).contains(&resolution) { + return Err( + res_token.as_external_error(BadGeoError::InvalidResolution(resolution)) + )?; + } + } - let xyz_base_point = lat_lng_to_xyz(&base_point); + let mut r1 = None; + if index.is_geo_filtering_enabled(rtxn)? { + let rtree = match index.geo_rtree(rtxn)? { + Some(rtree) => rtree, + None => return Ok(RoaringBitmap::new()), + }; - let result = rtree - .nearest_neighbor_iter(&xyz_base_point) - .take_while(|point| { - distance_between_two_points(&base_point, &point.data.1) - <= radius + f64::EPSILON - }) - .map(|point| point.data.0) - .collect(); + let xyz_base_point = lat_lng_to_xyz(&base_point); - Ok(result) + let result = rtree + .nearest_neighbor_iter(&xyz_base_point) + .take_while(|point| { + distance_between_two_points(&base_point, &point.data.1) + <= radius + f64::EPSILON + }) + .map(|point| point.data.0) + .collect(); + r1 = Some(result); + } + + let mut r2 = None; + if index.is_geojson_filtering_enabled(rtxn)? { + let point = geo_types::Point::new(base_point[1], base_point[0]); + + let result = index + .cellulite + .in_circle(rtxn, point, radius, resolution) + .map_err(InternalError::CelluliteError)?; + + r2 = Some(RoaringBitmap::from_iter(result)); // TODO: Remove once we update roaring + } + + match (r1, r2) { + (Some(r1), Some(r2)) => Ok(r1 | r2), + (Some(r1), None) => Ok(r1), + (None, Some(r2)) => Ok(r2), + (None, None) => { + Err(point[0].as_external_error(FilterError::AttributeNotFilterable { + attribute: &format!( + "{RESERVED_GEO_FIELD_NAME}/{RESERVED_GEOJSON_FIELD_NAME}" + ), + filterable_patterns: filtered_matching_patterns( + filterable_attribute_rules, + &|features| features.is_filterable(), + ), + }))? + } + } } FilterCondition::GeoBoundingBox { top_right_point, bottom_left_point } => { let top_right: [f64; 2] = [