From 64477aac60c82dafc572b6ad87b494fe5fe42732 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Renault?= Date: Tue, 1 Apr 2025 11:26:17 +0200 Subject: [PATCH] Box the large GeoError error variant --- crates/milli/src/error.rs | 4 +-- crates/milli/src/index.rs | 13 ++++--- .../src/update/index_documents/enrich.rs | 2 +- .../extract/extract_geo_points.rs | 14 +++++--- .../milli/src/update/new/extract/geo/mod.rs | 36 ++++++++++++------- 5 files changed, 45 insertions(+), 24 deletions(-) diff --git a/crates/milli/src/error.rs b/crates/milli/src/error.rs index e2f8fb6e4..e0d48e0ac 100644 --- a/crates/milli/src/error.rs +++ b/crates/milli/src/error.rs @@ -151,7 +151,7 @@ and can not be more than 511 bytes.", .document_id.to_string() matching_rule_indices: HashMap, }, #[error(transparent)] - InvalidGeoField(#[from] GeoError), + InvalidGeoField(#[from] Box), #[error("Invalid vector dimensions: expected: `{}`, found: `{}`.", .expected, .found)] InvalidVectorDimensions { expected: usize, found: usize }, #[error("The `_vectors` field in the document with id: `{document_id}` is not an object. Was expecting an object with a key for each embedder with manually provided vectors, but instead got `{value}`")] @@ -519,7 +519,7 @@ error_from_sub_error! { str::Utf8Error => InternalError, ThreadPoolBuildError => InternalError, SerializationError => InternalError, - GeoError => UserError, + Box => UserError, CriterionError => UserError, } diff --git a/crates/milli/src/index.rs b/crates/milli/src/index.rs index 771d32175..e2b6d857b 100644 --- a/crates/milli/src/index.rs +++ b/crates/milli/src/index.rs @@ -2954,10 +2954,15 @@ pub(crate) mod tests { documents!({ "id" : 6, RESERVED_GEO_FIELD_NAME: {"lat": "unparseable", "lng": "unparseable"}}), ) .unwrap_err(); - assert!(matches!( - err1, - Error::UserError(UserError::InvalidGeoField(GeoError::BadLatitudeAndLongitude { .. })) - )); + match err1 { + Error::UserError(UserError::InvalidGeoField(err)) => match *err { + GeoError::BadLatitudeAndLongitude { .. } => (), + otherwise => { + panic!("err1 is not a BadLatitudeAndLongitude error but rather a {otherwise:?}") + } + }, + _ => panic!("err1 is not a BadLatitudeAndLongitude error but rather a {err1:?}"), + } db_snap!(index, geo_faceted_documents_ids); // ensure that no more document was inserted } diff --git a/crates/milli/src/update/index_documents/enrich.rs b/crates/milli/src/update/index_documents/enrich.rs index 1f15dd570..0aaab70e8 100644 --- a/crates/milli/src/update/index_documents/enrich.rs +++ b/crates/milli/src/update/index_documents/enrich.rs @@ -115,7 +115,7 @@ pub fn enrich_documents_batch( if let Some(geo_value) = geo_field_id.and_then(|fid| document.get(fid)) { if let Err(user_error) = validate_geo_from_json(&document_id, geo_value)? { - return Ok(Err(UserError::from(user_error))); + return Ok(Err(UserError::from(Box::new(user_error)))); } } diff --git a/crates/milli/src/update/index_documents/extract/extract_geo_points.rs b/crates/milli/src/update/index_documents/extract/extract_geo_points.rs index 84f5e556b..fb2ea9d77 100644 --- a/crates/milli/src/update/index_documents/extract/extract_geo_points.rs +++ b/crates/milli/src/update/index_documents/extract/extract_geo_points.rs @@ -80,22 +80,28 @@ fn extract_lat_lng( let (lat, lng) = match (lat, lng) { (Some(lat), Some(lng)) => (lat, lng), (Some(_), None) => { - return Err(GeoError::MissingLatitude { document_id: document_id() }.into()) + return Err( + Box::new(GeoError::MissingLatitude { document_id: document_id() }).into() + ) } (None, Some(_)) => { - return Err(GeoError::MissingLongitude { document_id: document_id() }.into()) + return Err( + Box::new(GeoError::MissingLongitude { document_id: document_id() }).into() + ) } (None, None) => return Ok(None), }; let lat = extract_finite_float_from_value( serde_json::from_slice(lat).map_err(InternalError::SerdeJson)?, ) - .map_err(|lat| GeoError::BadLatitude { document_id: document_id(), value: lat })?; + .map_err(|lat| GeoError::BadLatitude { document_id: document_id(), value: lat }) + .map_err(Box::new)?; let lng = extract_finite_float_from_value( serde_json::from_slice(lng).map_err(InternalError::SerdeJson)?, ) - .map_err(|lng| GeoError::BadLongitude { document_id: document_id(), value: lng })?; + .map_err(|lng| GeoError::BadLongitude { document_id: document_id(), value: lng }) + .map_err(Box::new)?; Ok(Some([lat, lng])) } None => Ok(None), diff --git a/crates/milli/src/update/new/extract/geo/mod.rs b/crates/milli/src/update/new/extract/geo/mod.rs index 3d08298ab..b2ccc1b2b 100644 --- a/crates/milli/src/update/new/extract/geo/mod.rs +++ b/crates/milli/src/update/new/extract/geo/mod.rs @@ -258,9 +258,11 @@ pub fn extract_geo_coordinates( Value::Null => return Ok(None), Value::Object(map) => map, value => { - return Err( - GeoError::NotAnObject { document_id: Value::from(external_id), value }.into() - ) + return Err(Box::new(GeoError::NotAnObject { + document_id: Value::from(external_id), + value, + }) + .into()) } }; @@ -269,23 +271,29 @@ pub fn extract_geo_coordinates( if geo.is_empty() { [lat, lng] } else { - return Err(GeoError::UnexpectedExtraFields { + return Err(Box::new(GeoError::UnexpectedExtraFields { document_id: Value::from(external_id), value: Value::from(geo), - } + }) .into()); } } (Some(_), None) => { - return Err(GeoError::MissingLongitude { document_id: Value::from(external_id) }.into()) + return Err(Box::new(GeoError::MissingLongitude { + document_id: Value::from(external_id), + }) + .into()) } (None, Some(_)) => { - return Err(GeoError::MissingLatitude { document_id: Value::from(external_id) }.into()) + return Err(Box::new(GeoError::MissingLatitude { + document_id: Value::from(external_id), + }) + .into()) } (None, None) => { - return Err(GeoError::MissingLatitudeAndLongitude { + return Err(Box::new(GeoError::MissingLatitudeAndLongitude { document_id: Value::from(external_id), - } + }) .into()) } }; @@ -293,16 +301,18 @@ pub fn extract_geo_coordinates( match (extract_finite_float_from_value(lat), extract_finite_float_from_value(lng)) { (Ok(lat), Ok(lng)) => Ok(Some([lat, lng])), (Ok(_), Err(value)) => { - Err(GeoError::BadLongitude { document_id: Value::from(external_id), value }.into()) + Err(Box::new(GeoError::BadLongitude { document_id: Value::from(external_id), value }) + .into()) } (Err(value), Ok(_)) => { - Err(GeoError::BadLatitude { document_id: Value::from(external_id), value }.into()) + Err(Box::new(GeoError::BadLatitude { document_id: Value::from(external_id), value }) + .into()) } - (Err(lat), Err(lng)) => Err(GeoError::BadLatitudeAndLongitude { + (Err(lat), Err(lng)) => Err(Box::new(GeoError::BadLatitudeAndLongitude { document_id: Value::from(external_id), lat, lng, - } + }) .into()), } }