From a50b058557bbd77ec2a7038f887aa14c76c288f4 Mon Sep 17 00:00:00 2001 From: Tamo Date: Tue, 28 Mar 2023 18:26:18 +0200 Subject: [PATCH] update the geoBoundingBox feature Now instead of using the (top_left, bottom_right) corners of the bounding box it s using the (top_right, bottom_left) corners. --- filter-parser/src/lib.rs | 11 ++-- milli/src/index.rs | 24 +++++---- milli/src/search/facet/filter.rs | 86 +++++++++++++++++--------------- 3 files changed, 68 insertions(+), 53 deletions(-) diff --git a/filter-parser/src/lib.rs b/filter-parser/src/lib.rs index 8e21ff6be..d03d187fb 100644 --- a/filter-parser/src/lib.rs +++ b/filter-parser/src/lib.rs @@ -141,7 +141,7 @@ pub enum FilterCondition<'a> { Or(Vec), And(Vec), GeoLowerThan { point: [Token<'a>; 2], radius: Token<'a> }, - GeoBoundingBox { top_left_point: [Token<'a>; 2], bottom_right_point: [Token<'a>; 2] }, + GeoBoundingBox { top_right_point: [Token<'a>; 2], bottom_left_point: [Token<'a>; 2] }, } impl<'a> FilterCondition<'a> { @@ -362,8 +362,8 @@ fn parse_geo_bounding_box(input: Span) -> IResult { } let res = FilterCondition::GeoBoundingBox { - top_left_point: [args[0][0].into(), args[0][1].into()], - bottom_right_point: [args[1][0].into(), args[1][1].into()], + top_right_point: [args[0][0].into(), args[0][1].into()], + bottom_left_point: [args[1][0].into(), args[1][1].into()], }; Ok((input, res)) } @@ -780,7 +780,10 @@ impl<'a> std::fmt::Display for FilterCondition<'a> { FilterCondition::GeoLowerThan { point, radius } => { write!(f, "_geoRadius({}, {}, {})", point[0], point[1], radius) } - FilterCondition::GeoBoundingBox { top_left_point, bottom_right_point } => { + FilterCondition::GeoBoundingBox { + top_right_point: top_left_point, + bottom_left_point: bottom_right_point, + } => { write!( f, "_geoBoundingBox([{}, {}], [{}, {}])", diff --git a/milli/src/index.rs b/milli/src/index.rs index 20e64f984..acb2a0a9d 100644 --- a/milli/src/index.rs +++ b/milli/src/index.rs @@ -1586,35 +1586,35 @@ pub(crate) mod tests { // match a document in the middle of the rectangle let search_result = search - .filter(Filter::from_str("_geoBoundingBox([10, -10], [-10, 10])").unwrap().unwrap()) + .filter(Filter::from_str("_geoBoundingBox([10, 10], [-10, -10])").unwrap().unwrap()) .execute() .unwrap(); insta::assert_debug_snapshot!(search_result.candidates, @"RoaringBitmap<[0]>"); // select everything let search_result = search - .filter(Filter::from_str("_geoBoundingBox([90, -180], [-90, 180])").unwrap().unwrap()) + .filter(Filter::from_str("_geoBoundingBox([90, 180], [-90, -180])").unwrap().unwrap()) .execute() .unwrap(); insta::assert_debug_snapshot!(search_result.candidates, @"RoaringBitmap<[0, 1, 2, 3, 4]>"); // go on the edge of the longitude let search_result = search - .filter(Filter::from_str("_geoBoundingBox([0, 180], [0, -170])").unwrap().unwrap()) + .filter(Filter::from_str("_geoBoundingBox([0, -170], [0, 180])").unwrap().unwrap()) .execute() .unwrap(); insta::assert_debug_snapshot!(search_result.candidates, @"RoaringBitmap<[1]>"); // go on the other edge of the longitude let search_result = search - .filter(Filter::from_str("_geoBoundingBox([0, 170], [0, -180])").unwrap().unwrap()) + .filter(Filter::from_str("_geoBoundingBox([0, -180], [0, 170])").unwrap().unwrap()) .execute() .unwrap(); insta::assert_debug_snapshot!(search_result.candidates, @"RoaringBitmap<[2]>"); // wrap around the longitude let search_result = search - .filter(Filter::from_str("_geoBoundingBox([0, 170], [0, -170])").unwrap().unwrap()) + .filter(Filter::from_str("_geoBoundingBox([0, -170], [0, 170])").unwrap().unwrap()) .execute() .unwrap(); insta::assert_debug_snapshot!(search_result.candidates, @"RoaringBitmap<[1, 2]>"); @@ -1640,20 +1640,26 @@ pub(crate) mod tests { .filter(Filter::from_str("_geoBoundingBox([-80, 0], [80, 0])").unwrap().unwrap()) .execute() .unwrap_err(); - insta::assert_display_snapshot!(error, @r###" + insta::assert_display_snapshot!( + error, + @r###" The top latitude `-80` is below the bottom latitude `80`. 32:33 _geoBoundingBox([-80, 0], [80, 0]) - "###); + "### + ); // send a top latitude lower than the bottow latitude let error = search .filter(Filter::from_str("_geoBoundingBox([-10, 0], [10, 0])").unwrap().unwrap()) .execute() .unwrap_err(); - insta::assert_display_snapshot!(error, @r###" + insta::assert_display_snapshot!( + error, + @r###" The top latitude `-10` is below the bottom latitude `10`. 32:33 _geoBoundingBox([-10, 0], [10, 0]) - "###); + "### + ); } #[test] diff --git a/milli/src/search/facet/filter.rs b/milli/src/search/facet/filter.rs index a4ac53950..f67219494 100644 --- a/milli/src/search/facet/filter.rs +++ b/milli/src/search/facet/filter.rs @@ -419,54 +419,56 @@ impl<'a> Filter<'a> { }))? } } - FilterCondition::GeoBoundingBox { top_left_point, bottom_right_point } => { + FilterCondition::GeoBoundingBox { top_right_point, bottom_left_point } => { if filterable_fields.contains("_geo") { - let top_left: [f64; 2] = [ - top_left_point[0].parse_finite_float()?, - top_left_point[1].parse_finite_float()?, + let top_right: [f64; 2] = [ + top_right_point[0].parse_finite_float()?, + top_right_point[1].parse_finite_float()?, ]; - let bottom_right: [f64; 2] = [ - bottom_right_point[0].parse_finite_float()?, - bottom_right_point[1].parse_finite_float()?, + let bottom_left: [f64; 2] = [ + bottom_left_point[0].parse_finite_float()?, + bottom_left_point[1].parse_finite_float()?, ]; - if !(-90.0..=90.0).contains(&top_left[0]) { + if !(-90.0..=90.0).contains(&top_right[0]) { return Err( - top_left_point[0].as_external_error(BadGeoError::Lat(top_left[0])) + top_right_point[0].as_external_error(BadGeoError::Lat(top_right[0])) )?; } - if !(-180.0..=180.0).contains(&top_left[1]) { + if !(-180.0..=180.0).contains(&top_right[1]) { return Err( - top_left_point[1].as_external_error(BadGeoError::Lng(top_left[1])) + top_right_point[1].as_external_error(BadGeoError::Lng(top_right[1])) )?; } - if !(-90.0..=90.0).contains(&bottom_right[0]) { - return Err(bottom_right_point[0] - .as_external_error(BadGeoError::Lat(bottom_right[0])))?; + if !(-90.0..=90.0).contains(&bottom_left[0]) { + return Err(bottom_left_point[0] + .as_external_error(BadGeoError::Lat(bottom_left[0])))?; } - if !(-180.0..=180.0).contains(&bottom_right[1]) { - return Err(bottom_right_point[1] - .as_external_error(BadGeoError::Lng(bottom_right[1])))?; + if !(-180.0..=180.0).contains(&bottom_left[1]) { + return Err(bottom_left_point[1] + .as_external_error(BadGeoError::Lng(bottom_left[1])))?; } - if top_left[0] < bottom_right[0] { - return Err(bottom_right_point[1].as_external_error( - BadGeoError::BoundingBoxTopIsBelowBottom(top_left[0], bottom_right[0]), + if top_right[0] < bottom_left[0] { + return Err(bottom_left_point[1].as_external_error( + BadGeoError::BoundingBoxTopIsBelowBottom(top_right[0], bottom_left[0]), ))?; } // Instead of writing a custom `GeoBoundingBox` filter we're simply going to re-use the range // filter to create the following filter; - // `_geo.lat {top_left[0]} TO {bottom_right[0]} AND _geo.lng {top_left[1]} TO {bottom_right[1]}` + // `_geo.lat {top_right[0]} TO {bottom_left[0]} AND _geo.lng {top_right[1]} TO {bottom_left[1]}` // As we can see, we need to use a bunch of tokens that don't exist in the original filter, // thus we're going to create tokens that point to a random span but contain our text. - let geo_lat_token = - Token::new(top_left_point[0].original_span(), Some("_geo.lat".to_string())); + let geo_lat_token = Token::new( + top_right_point[0].original_span(), + Some("_geo.lat".to_string()), + ); let condition_lat = FilterCondition::Condition { fid: geo_lat_token, op: Condition::Between { - from: bottom_right_point[0].clone(), - to: top_left_point[0].clone(), + from: bottom_left_point[0].clone(), + to: top_right_point[0].clone(), }, }; @@ -476,27 +478,29 @@ impl<'a> Filter<'a> { filterable_fields, )?; - let geo_lng_token = - Token::new(top_left_point[1].original_span(), Some("_geo.lng".to_string())); - let selected_lng = if top_left[1] > bottom_right[1] { + let geo_lng_token = Token::new( + top_right_point[1].original_span(), + Some("_geo.lng".to_string()), + ); + let selected_lng = if top_right[1] < bottom_left[1] { // In this case the bounding box is wrapping around the earth (going from 180 to -180). // We need to update the lng part of the filter from; - // `_geo.lng {top_left[1]} TO {bottom_right[1]}` to - // `_geo.lng {top_left[1]} TO 180 AND _geo.lng -180 TO {bottom_right[1]}` + // `_geo.lng {top_right[1]} TO {bottom_left[1]}` to + // `_geo.lng {bottom_left[1]} TO 180 AND _geo.lng -180 TO {top_right[1]}` let min_lng_token = Token::new( - top_left_point[1].original_span(), + top_right_point[1].original_span(), Some("-180.0".to_string()), ); let max_lng_token = Token::new( - top_left_point[1].original_span(), + top_right_point[1].original_span(), Some("180.0".to_string()), ); let condition_left = FilterCondition::Condition { fid: geo_lng_token.clone(), op: Condition::Between { - from: top_left_point[1].clone(), + from: bottom_left_point[1].clone(), to: max_lng_token, }, }; @@ -510,7 +514,7 @@ impl<'a> Filter<'a> { fid: geo_lng_token, op: Condition::Between { from: min_lng_token, - to: bottom_right_point[1].clone(), + to: top_right_point[1].clone(), }, }; let right = Filter { condition: condition_right }.inner_evaluate( @@ -524,8 +528,8 @@ impl<'a> Filter<'a> { let condition_lng = FilterCondition::Condition { fid: geo_lng_token, op: Condition::Between { - from: top_left_point[1].clone(), - to: bottom_right_point[1].clone(), + from: bottom_left_point[1].clone(), + to: top_right_point[1].clone(), }, }; Filter { condition: condition_lng }.inner_evaluate( @@ -537,10 +541,12 @@ impl<'a> Filter<'a> { Ok(selected_lat & selected_lng) } else { - Err(top_left_point[0].as_external_error(FilterError::AttributeNotFilterable { - attribute: "_geo", - filterable_fields: filterable_fields.clone(), - }))? + Err(top_right_point[0].as_external_error( + FilterError::AttributeNotFilterable { + attribute: "_geo", + filterable_fields: filterable_fields.clone(), + }, + ))? } } }