Merge #3993

3993: Bringing back changes from v1.3.1 to `main` r=irevoire a=curquiza



Co-authored-by: irevoire <irevoire@users.noreply.github.com>
Co-authored-by: meili-bors[bot] <89034592+meili-bors[bot]@users.noreply.github.com>
Co-authored-by: Tamo <tamo@meilisearch.com>
Co-authored-by: ManyTheFish <many@meilisearch.com>

milli/src/index.rs

@@ -1820,11 +1820,11 @@ pub(crate) mod tests {
             .unwrap();
         index
             .add_documents(documents!([
-                { "id": 0, "_geo": { "lat": 0, "lng": 0 } },
-                { "id": 1, "_geo": { "lat": 0, "lng": -175 } },
-                { "id": 2, "_geo": { "lat": 0, "lng": 175 } },
+                { "id": 0, "_geo": { "lat": "0", "lng": "0" } },
+                { "id": 1, "_geo": { "lat": 0, "lng": "-175" } },
+                { "id": 2, "_geo": { "lat": "0", "lng": 175 } },
                 { "id": 3, "_geo": { "lat": 85, "lng": 0 } },
-                { "id": 4, "_geo": { "lat": -85, "lng": 0 } },
+                { "id": 4, "_geo": { "lat": "-85", "lng": "0" } },
             ]))
             .unwrap();
 

milli/src/lib.rs

@@ -97,7 +97,7 @@ const MAX_LMDB_KEY_LENGTH: usize = 500;
 ///
 /// This number is determined by the keys of the different facet databases
 /// and adding a margin of safety.
-pub const MAX_FACET_VALUE_LENGTH: usize = MAX_LMDB_KEY_LENGTH - 20;
+pub const MAX_FACET_VALUE_LENGTH: usize = MAX_LMDB_KEY_LENGTH - 32;
 
 /// The maximum length a word can be
 pub const MAX_WORD_LENGTH: usize = MAX_LMDB_KEY_LENGTH / 2;

milli/src/update/facet/mod.rs

@@ -94,7 +94,7 @@ use crate::heed_codec::facet::{FacetGroupKey, FacetGroupKeyCodec, FacetGroupValu
 use crate::heed_codec::ByteSliceRefCodec;
 use crate::update::index_documents::create_sorter;
 use crate::update::merge_btreeset_string;
-use crate::{BEU16StrCodec, Index, Result, BEU16};
+use crate::{BEU16StrCodec, Index, Result, BEU16, MAX_FACET_VALUE_LENGTH};
 
 pub mod bulk;
 pub mod delete;
@@ -191,7 +191,16 @@ impl<'i> FacetsUpdate<'i> {
         for result in database.iter(wtxn)? {
             let (facet_group_key, ()) = result?;
             if let FacetGroupKey { field_id, level: 0, left_bound } = facet_group_key {
-                let normalized_facet = left_bound.normalize(&options);
+                let mut normalized_facet = left_bound.normalize(&options);
+                let normalized_truncated_facet: String;
+                if normalized_facet.len() > MAX_FACET_VALUE_LENGTH {
+                    normalized_truncated_facet = normalized_facet
+                        .char_indices()
+                        .take_while(|(idx, _)| *idx < MAX_FACET_VALUE_LENGTH)
+                        .map(|(_, c)| c)
+                        .collect();
+                    normalized_facet = normalized_truncated_facet.into();
+                }
                 let set = BTreeSet::from_iter(std::iter::once(left_bound));
                 let key = (field_id, normalized_facet.as_ref());
                 let key = BEU16StrCodec::bytes_encode(&key).ok_or(heed::Error::Encoding)?;

milli/src/update/index_documents/extract/extract_facet_string_docids.rs

@@ -46,7 +46,7 @@ pub fn extract_facet_string_docids<R: io::Read + io::Seek>(
         if normalised_value.len() > MAX_FACET_VALUE_LENGTH {
             normalised_truncated_value = normalised_value
                 .char_indices()
-                .take_while(|(idx, _)| idx + 4 < MAX_FACET_VALUE_LENGTH)
+                .take_while(|(idx, _)| *idx < MAX_FACET_VALUE_LENGTH)
                 .map(|(_, c)| c)
                 .collect();
             normalised_value = normalised_truncated_value.as_str();

milli/src/update/index_documents/extract/extract_fid_docid_facet_values.rs

@@ -28,11 +28,13 @@ pub struct ExtractedFacetValues {
 ///
 /// Returns the generated grenad reader containing the docid the fid and the orginal value as key
 /// and the normalized value as value extracted from the given chunk of documents.
+/// We need the fid of the geofields to correctly parse them as numbers if they were sent as strings initially.
 #[logging_timer::time]
 pub fn extract_fid_docid_facet_values<R: io::Read + io::Seek>(
     obkv_documents: grenad::Reader<R>,
     indexer: GrenadParameters,
     faceted_fields: &HashSet<FieldId>,
+    geo_fields_ids: Option<(FieldId, FieldId)>,
 ) -> Result<ExtractedFacetValues> {
     puffin::profile_function!();
 
@@ -84,7 +86,10 @@ pub fn extract_fid_docid_facet_values<R: io::Read + io::Seek>(
 
                 let value = from_slice(field_bytes).map_err(InternalError::SerdeJson)?;
 
-                match extract_facet_values(&value) {
+                match extract_facet_values(
+                    &value,
+                    geo_fields_ids.map_or(false, |(lat, lng)| field_id == lat || field_id == lng),
+                ) {
                     FilterableValues::Null => {
                         facet_is_null_docids.entry(field_id).or_default().insert(document);
                     }
@@ -177,12 +182,13 @@ enum FilterableValues {
     Values { numbers: Vec<f64>, strings: Vec<(String, String)> },
 }
 
-fn extract_facet_values(value: &Value) -> FilterableValues {
+fn extract_facet_values(value: &Value, geo_field: bool) -> FilterableValues {
     fn inner_extract_facet_values(
         value: &Value,
         can_recurse: bool,
         output_numbers: &mut Vec<f64>,
         output_strings: &mut Vec<(String, String)>,
+        geo_field: bool,
     ) {
         match value {
             Value::Null => (),
@@ -193,13 +199,30 @@ fn extract_facet_values(value: &Value) -> FilterableValues {
                 }
             }
             Value::String(original) => {
+                // if we're working on a geofield it MUST be something we can parse or else there was an internal error
+                // in the enrich pipeline. But since the enrich pipeline worked, we want to avoid crashing at all costs.
+                if geo_field {
+                    if let Ok(float) = original.parse() {
+                        output_numbers.push(float);
+                    } else {
+                        log::warn!(
+                            "Internal error, could not parse a geofield that has been validated. Please open an issue."
+                        )
+                    }
+                }
                 let normalized = crate::normalize_facet(original);
                 output_strings.push((normalized, original.clone()));
             }
             Value::Array(values) => {
                 if can_recurse {
                     for value in values {
-                        inner_extract_facet_values(value, false, output_numbers, output_strings);
+                        inner_extract_facet_values(
+                            value,
+                            false,
+                            output_numbers,
+                            output_strings,
+                            geo_field,
+                        );
                     }
                 }
             }
@@ -215,7 +238,7 @@ fn extract_facet_values(value: &Value) -> FilterableValues {
         otherwise => {
             let mut numbers = Vec::new();
             let mut strings = Vec::new();
-            inner_extract_facet_values(otherwise, true, &mut numbers, &mut strings);
+            inner_extract_facet_values(otherwise, true, &mut numbers, &mut strings, geo_field);
             FilterableValues::Values { numbers, strings }
         }
     }

milli/src/update/index_documents/extract/mod.rs

@@ -378,6 +378,7 @@ fn send_and_extract_flattened_documents_data(
                     flattened_documents_chunk.clone(),
                     indexer,
                     faceted_fields,
+                    geo_fields_ids,
                 )?;
 
                 // send docid_fid_facet_numbers_chunk to DB writer