From 24e94b28c1c4ae30e4d3256f3af896b8446ba722 Mon Sep 17 00:00:00 2001 From: nnethercott Date: Mon, 26 May 2025 09:22:20 +0200 Subject: [PATCH 01/10] feat: uncouple geo extraction from full doc --- .../new/extract/faceted/extract_facets.rs | 248 ++++++++++++------ .../new/extract/faceted/facet_document.rs | 29 +- 2 files changed, 188 insertions(+), 89 deletions(-) diff --git a/crates/milli/src/update/new/extract/faceted/extract_facets.rs b/crates/milli/src/update/new/extract/faceted/extract_facets.rs index 01cfe338f..aa7510863 100644 --- a/crates/milli/src/update/new/extract/faceted/extract_facets.rs +++ b/crates/milli/src/update/new/extract/faceted/extract_facets.rs @@ -8,7 +8,7 @@ use hashbrown::HashMap; use serde_json::Value; use super::super::cache::BalancedCaches; -use super::facet_document::extract_document_facets; +use super::facet_document::{extract_document_facets, extract_geo_document}; use super::FacetKind; use crate::fields_ids_map::metadata::Metadata; use crate::filterable_attributes_rules::match_faceted_field; @@ -90,53 +90,8 @@ impl FacetedDocidsExtractor { let mut cached_sorter = context.data.borrow_mut_or_yield(); let mut del_add_facet_value = DelAddFacetValue::new(&context.doc_alloc); let docid = document_change.docid(); - let res = match document_change { - DocumentChange::Deletion(inner) => extract_document_facets( - inner.current(rtxn, index, context.db_fields_ids_map)?, - inner.external_document_id(), - new_fields_ids_map.deref_mut(), - filterable_attributes, - sortable_fields, - asc_desc_fields, - distinct_field, - is_geo_enabled, - &mut |fid, meta, depth, value| { - Self::facet_fn_with_options( - &context.doc_alloc, - cached_sorter.deref_mut(), - BalancedCaches::insert_del_u32, - &mut del_add_facet_value, - DelAddFacetValue::insert_del, - docid, - fid, - meta, - filterable_attributes, - depth, - value, - ) - }, - ), - DocumentChange::Update(inner) => { - let has_changed = inner.has_changed_for_fields( - &mut |field_name| { - match_faceted_field( - field_name, - filterable_attributes, - sortable_fields, - asc_desc_fields, - distinct_field, - ) - }, - rtxn, - index, - context.db_fields_ids_map, - )?; - let has_changed_for_geo_fields = - inner.has_changed_for_geo_fields(rtxn, index, context.db_fields_ids_map)?; - if !has_changed && !has_changed_for_geo_fields { - return Ok(()); - } - + match document_change { + DocumentChange::Deletion(inner) => { extract_document_facets( inner.current(rtxn, index, context.db_fields_ids_map)?, inner.external_document_id(), @@ -145,7 +100,6 @@ impl FacetedDocidsExtractor { sortable_fields, asc_desc_fields, distinct_field, - is_geo_enabled, &mut |fid, meta, depth, value| { Self::facet_fn_with_options( &context.doc_alloc, @@ -163,15 +117,155 @@ impl FacetedDocidsExtractor { }, )?; + if is_geo_enabled { + extract_geo_document( + inner.current(rtxn, index, context.db_fields_ids_map)?, + inner.external_document_id(), + new_fields_ids_map.deref_mut(), + asc_desc_fields, + &mut |fid, meta, depth, value| { + Self::facet_fn_with_options( + &context.doc_alloc, + cached_sorter.deref_mut(), + BalancedCaches::insert_del_u32, + &mut del_add_facet_value, + DelAddFacetValue::insert_del, + docid, + fid, + meta, + filterable_attributes, + depth, + value, + ) + }, + )?; + } + } + DocumentChange::Update(inner) => { + let has_changed = inner.has_changed_for_fields( + &mut |field_name| { + match_faceted_field( + field_name, + filterable_attributes, + sortable_fields, + asc_desc_fields, + distinct_field, + ) + }, + rtxn, + index, + context.db_fields_ids_map, + )?; + let has_changed_for_geo_fields = + inner.has_changed_for_geo_fields(rtxn, index, context.db_fields_ids_map)?; + + if has_changed { + extract_document_facets( + inner.current(rtxn, index, context.db_fields_ids_map)?, + inner.external_document_id(), + new_fields_ids_map.deref_mut(), + filterable_attributes, + sortable_fields, + asc_desc_fields, + distinct_field, + &mut |fid, meta, depth, value| { + Self::facet_fn_with_options( + &context.doc_alloc, + cached_sorter.deref_mut(), + BalancedCaches::insert_del_u32, + &mut del_add_facet_value, + DelAddFacetValue::insert_del, + docid, + fid, + meta, + filterable_attributes, + depth, + value, + ) + }, + )?; + + extract_document_facets( + inner.merged(rtxn, index, context.db_fields_ids_map)?, + inner.external_document_id(), + new_fields_ids_map.deref_mut(), + filterable_attributes, + sortable_fields, + asc_desc_fields, + distinct_field, + &mut |fid, meta, depth, value| { + Self::facet_fn_with_options( + &context.doc_alloc, + cached_sorter.deref_mut(), + BalancedCaches::insert_add_u32, + &mut del_add_facet_value, + DelAddFacetValue::insert_add, + docid, + fid, + meta, + filterable_attributes, + depth, + value, + ) + }, + )?; + } + + if is_geo_enabled && has_changed_for_geo_fields{ + extract_geo_document( + inner.current(rtxn, index, context.db_fields_ids_map)?, + inner.external_document_id(), + new_fields_ids_map.deref_mut(), + asc_desc_fields, + &mut |fid, meta, depth, value| { + Self::facet_fn_with_options( + &context.doc_alloc, + cached_sorter.deref_mut(), + BalancedCaches::insert_del_u32, + &mut del_add_facet_value, + DelAddFacetValue::insert_del, + docid, + fid, + meta, + filterable_attributes, + depth, + value, + ) + }, + )?; + + extract_geo_document( + inner.merged(rtxn, index, context.db_fields_ids_map)?, + inner.external_document_id(), + new_fields_ids_map.deref_mut(), + asc_desc_fields, + &mut |fid, meta, depth, value| { + Self::facet_fn_with_options( + &context.doc_alloc, + cached_sorter.deref_mut(), + BalancedCaches::insert_add_u32, + &mut del_add_facet_value, + DelAddFacetValue::insert_add, + docid, + fid, + meta, + filterable_attributes, + depth, + value, + ) + }, + )?; + } + } + DocumentChange::Insertion(inner) => { extract_document_facets( - inner.merged(rtxn, index, context.db_fields_ids_map)?, + inner.inserted(), inner.external_document_id(), new_fields_ids_map.deref_mut(), filterable_attributes, sortable_fields, asc_desc_fields, distinct_field, - is_geo_enabled, &mut |fid, meta, depth, value| { Self::facet_fn_with_options( &context.doc_alloc, @@ -187,37 +281,35 @@ impl FacetedDocidsExtractor { value, ) }, - ) + )?; + if is_geo_enabled { + extract_geo_document( + inner.inserted(), + inner.external_document_id(), + new_fields_ids_map.deref_mut(), + asc_desc_fields, + &mut |fid, meta, depth, value| { + Self::facet_fn_with_options( + &context.doc_alloc, + cached_sorter.deref_mut(), + BalancedCaches::insert_add_u32, + &mut del_add_facet_value, + DelAddFacetValue::insert_add, + docid, + fid, + meta, + filterable_attributes, + depth, + value, + ) + }, + )?; + } } - DocumentChange::Insertion(inner) => extract_document_facets( - inner.inserted(), - inner.external_document_id(), - new_fields_ids_map.deref_mut(), - filterable_attributes, - sortable_fields, - asc_desc_fields, - distinct_field, - is_geo_enabled, - &mut |fid, meta, depth, value| { - Self::facet_fn_with_options( - &context.doc_alloc, - cached_sorter.deref_mut(), - BalancedCaches::insert_add_u32, - &mut del_add_facet_value, - DelAddFacetValue::insert_add, - docid, - fid, - meta, - filterable_attributes, - depth, - value, - ) - }, - ), }; del_add_facet_value.send_data(docid, sender, &context.doc_alloc).unwrap(); - res + Ok(()) } #[allow(clippy::too_many_arguments)] diff --git a/crates/milli/src/update/new/extract/faceted/facet_document.rs b/crates/milli/src/update/new/extract/faceted/facet_document.rs index e74131402..30a5c462e 100644 --- a/crates/milli/src/update/new/extract/faceted/facet_document.rs +++ b/crates/milli/src/update/new/extract/faceted/facet_document.rs @@ -22,7 +22,6 @@ pub fn extract_document_facets<'doc>( sortable_fields: &HashSet, asc_desc_fields: &HashSet, distinct_field: &Option, - is_geo_enabled: bool, facet_fn: &mut impl FnMut(FieldId, Metadata, perm_json_p::Depth, &Value) -> Result<()>, ) -> Result<()> { // return the match result for the given field name. @@ -102,17 +101,25 @@ pub fn extract_document_facets<'doc>( } } - if is_geo_enabled { - if let Some(geo_value) = document.geo_field()? { - if let Some([lat, lng]) = extract_geo_coordinates(external_document_id, geo_value)? { - let ((lat_fid, lat_meta), (lng_fid, lng_meta)) = field_id_map - .id_with_metadata_or_insert("_geo.lat") - .zip(field_id_map.id_with_metadata_or_insert("_geo.lng")) - .ok_or(UserError::AttributeLimitReached)?; + Ok(()) +} - facet_fn(lat_fid, lat_meta, perm_json_p::Depth::OnBaseKey, &lat.into())?; - facet_fn(lng_fid, lng_meta, perm_json_p::Depth::OnBaseKey, &lng.into())?; - } +pub fn extract_geo_document<'doc>( + document: impl Document<'doc>, + external_document_id: &str, + field_id_map: &mut GlobalFieldsIdsMap, + asc_desc_fields: &HashSet, + facet_fn: &mut impl FnMut(FieldId, Metadata, perm_json_p::Depth, &Value) -> Result<()>, +) -> Result<()> { + if let Some(geo_value) = document.geo_field()? { + if let Some([lat, lng]) = extract_geo_coordinates(external_document_id, geo_value)? { + let ((lat_fid, lat_meta), (lng_fid, lng_meta)) = field_id_map + .id_with_metadata_or_insert("_geo.lat") + .zip(field_id_map.id_with_metadata_or_insert("_geo.lng")) + .ok_or(UserError::AttributeLimitReached)?; + + facet_fn(lat_fid, lat_meta, perm_json_p::Depth::OnBaseKey, &lat.into())?; + facet_fn(lng_fid, lng_meta, perm_json_p::Depth::OnBaseKey, &lng.into())?; } } From f690fa068631e8f324c37e1a7aeccb69d642288e Mon Sep 17 00:00:00 2001 From: nnethercott Date: Mon, 26 May 2025 09:46:14 +0200 Subject: [PATCH 02/10] feat: add macro_rules to factorize --- .../new/extract/faceted/extract_facets.rs | 207 ++++++------------ 1 file changed, 72 insertions(+), 135 deletions(-) diff --git a/crates/milli/src/update/new/extract/faceted/extract_facets.rs b/crates/milli/src/update/new/extract/faceted/extract_facets.rs index aa7510863..241e0fd69 100644 --- a/crates/milli/src/update/new/extract/faceted/extract_facets.rs +++ b/crates/milli/src/update/new/extract/faceted/extract_facets.rs @@ -90,8 +90,48 @@ impl FacetedDocidsExtractor { let mut cached_sorter = context.data.borrow_mut_or_yield(); let mut del_add_facet_value = DelAddFacetValue::new(&context.doc_alloc); let docid = document_change.docid(); + + macro_rules! facet_fn_factory { + (del) => { + |fid: FieldId, meta: Metadata, depth: perm_json_p::Depth, value: &Value| { + Self::facet_fn_with_options( + &context.doc_alloc, + cached_sorter.deref_mut(), + BalancedCaches::insert_del_u32, + &mut del_add_facet_value, + DelAddFacetValue::insert_del, + docid, + fid, + meta, + filterable_attributes, + depth, + value, + ) + } + }; + (add) => { + |fid: FieldId, meta: Metadata, depth: perm_json_p::Depth, value: &Value| { + Self::facet_fn_with_options( + &context.doc_alloc, + cached_sorter.deref_mut(), + BalancedCaches::insert_add_u32, + &mut del_add_facet_value, + DelAddFacetValue::insert_add, + docid, + fid, + meta, + filterable_attributes, + depth, + value, + ) + } + }; + } + match document_change { DocumentChange::Deletion(inner) => { + let mut del = facet_fn_factory!(del); + extract_document_facets( inner.current(rtxn, index, context.db_fields_ids_map)?, inner.external_document_id(), @@ -100,21 +140,7 @@ impl FacetedDocidsExtractor { sortable_fields, asc_desc_fields, distinct_field, - &mut |fid, meta, depth, value| { - Self::facet_fn_with_options( - &context.doc_alloc, - cached_sorter.deref_mut(), - BalancedCaches::insert_del_u32, - &mut del_add_facet_value, - DelAddFacetValue::insert_del, - docid, - fid, - meta, - filterable_attributes, - depth, - value, - ) - }, + &mut del, )?; if is_geo_enabled { @@ -123,21 +149,7 @@ impl FacetedDocidsExtractor { inner.external_document_id(), new_fields_ids_map.deref_mut(), asc_desc_fields, - &mut |fid, meta, depth, value| { - Self::facet_fn_with_options( - &context.doc_alloc, - cached_sorter.deref_mut(), - BalancedCaches::insert_del_u32, - &mut del_add_facet_value, - DelAddFacetValue::insert_del, - docid, - fid, - meta, - filterable_attributes, - depth, - value, - ) - }, + &mut del, )?; } } @@ -160,6 +172,9 @@ impl FacetedDocidsExtractor { inner.has_changed_for_geo_fields(rtxn, index, context.db_fields_ids_map)?; if has_changed { + // 1. Delete old facet values + let mut del = facet_fn_factory!(del); + extract_document_facets( inner.current(rtxn, index, context.db_fields_ids_map)?, inner.external_document_id(), @@ -168,23 +183,21 @@ impl FacetedDocidsExtractor { sortable_fields, asc_desc_fields, distinct_field, - &mut |fid, meta, depth, value| { - Self::facet_fn_with_options( - &context.doc_alloc, - cached_sorter.deref_mut(), - BalancedCaches::insert_del_u32, - &mut del_add_facet_value, - DelAddFacetValue::insert_del, - docid, - fid, - meta, - filterable_attributes, - depth, - value, - ) - }, + &mut del, )?; + if is_geo_enabled && has_changed_for_geo_fields { + extract_geo_document( + inner.current(rtxn, index, context.db_fields_ids_map)?, + inner.external_document_id(), + new_fields_ids_map.deref_mut(), + asc_desc_fields, + &mut del, + )?; + } + + let mut add = facet_fn_factory!(add); + extract_document_facets( inner.merged(rtxn, index, context.db_fields_ids_map)?, inner.external_document_id(), @@ -193,71 +206,23 @@ impl FacetedDocidsExtractor { sortable_fields, asc_desc_fields, distinct_field, - &mut |fid, meta, depth, value| { - Self::facet_fn_with_options( - &context.doc_alloc, - cached_sorter.deref_mut(), - BalancedCaches::insert_add_u32, - &mut del_add_facet_value, - DelAddFacetValue::insert_add, - docid, - fid, - meta, - filterable_attributes, - depth, - value, - ) - }, - )?; - } - - if is_geo_enabled && has_changed_for_geo_fields{ - extract_geo_document( - inner.current(rtxn, index, context.db_fields_ids_map)?, - inner.external_document_id(), - new_fields_ids_map.deref_mut(), - asc_desc_fields, - &mut |fid, meta, depth, value| { - Self::facet_fn_with_options( - &context.doc_alloc, - cached_sorter.deref_mut(), - BalancedCaches::insert_del_u32, - &mut del_add_facet_value, - DelAddFacetValue::insert_del, - docid, - fid, - meta, - filterable_attributes, - depth, - value, - ) - }, + &mut add, )?; - extract_geo_document( - inner.merged(rtxn, index, context.db_fields_ids_map)?, - inner.external_document_id(), - new_fields_ids_map.deref_mut(), - asc_desc_fields, - &mut |fid, meta, depth, value| { - Self::facet_fn_with_options( - &context.doc_alloc, - cached_sorter.deref_mut(), - BalancedCaches::insert_add_u32, - &mut del_add_facet_value, - DelAddFacetValue::insert_add, - docid, - fid, - meta, - filterable_attributes, - depth, - value, - ) - }, - )?; + if is_geo_enabled && has_changed_for_geo_fields { + extract_geo_document( + inner.merged(rtxn, index, context.db_fields_ids_map)?, + inner.external_document_id(), + new_fields_ids_map.deref_mut(), + asc_desc_fields, + &mut add, + )?; + } } } DocumentChange::Insertion(inner) => { + let mut add = facet_fn_factory!(add); + extract_document_facets( inner.inserted(), inner.external_document_id(), @@ -266,21 +231,7 @@ impl FacetedDocidsExtractor { sortable_fields, asc_desc_fields, distinct_field, - &mut |fid, meta, depth, value| { - Self::facet_fn_with_options( - &context.doc_alloc, - cached_sorter.deref_mut(), - BalancedCaches::insert_add_u32, - &mut del_add_facet_value, - DelAddFacetValue::insert_add, - docid, - fid, - meta, - filterable_attributes, - depth, - value, - ) - }, + &mut add, )?; if is_geo_enabled { extract_geo_document( @@ -288,21 +239,7 @@ impl FacetedDocidsExtractor { inner.external_document_id(), new_fields_ids_map.deref_mut(), asc_desc_fields, - &mut |fid, meta, depth, value| { - Self::facet_fn_with_options( - &context.doc_alloc, - cached_sorter.deref_mut(), - BalancedCaches::insert_add_u32, - &mut del_add_facet_value, - DelAddFacetValue::insert_add, - docid, - fid, - meta, - filterable_attributes, - depth, - value, - ) - }, + &mut add, )?; } } From 95821d0bdebed6e9762183ddd46cce6d4c453adb Mon Sep 17 00:00:00 2001 From: nnethercott Date: Mon, 26 May 2025 10:07:13 +0200 Subject: [PATCH 03/10] refactor: update macro --- .../update/new/extract/faceted/extract_facets.rs | 16 +++++++--------- .../update/new/extract/faceted/facet_document.rs | 1 - 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/crates/milli/src/update/new/extract/faceted/extract_facets.rs b/crates/milli/src/update/new/extract/faceted/extract_facets.rs index 241e0fd69..de0edc164 100644 --- a/crates/milli/src/update/new/extract/faceted/extract_facets.rs +++ b/crates/milli/src/update/new/extract/faceted/extract_facets.rs @@ -91,7 +91,8 @@ impl FacetedDocidsExtractor { let mut del_add_facet_value = DelAddFacetValue::new(&context.doc_alloc); let docid = document_change.docid(); - macro_rules! facet_fn_factory { + // Macro expanding to an insertion/deletion facet fn + macro_rules! facet_fn { (del) => { |fid: FieldId, meta: Metadata, depth: perm_json_p::Depth, value: &Value| { Self::facet_fn_with_options( @@ -130,7 +131,7 @@ impl FacetedDocidsExtractor { match document_change { DocumentChange::Deletion(inner) => { - let mut del = facet_fn_factory!(del); + let mut del = facet_fn!(del); extract_document_facets( inner.current(rtxn, index, context.db_fields_ids_map)?, @@ -148,7 +149,6 @@ impl FacetedDocidsExtractor { inner.current(rtxn, index, context.db_fields_ids_map)?, inner.external_document_id(), new_fields_ids_map.deref_mut(), - asc_desc_fields, &mut del, )?; } @@ -173,7 +173,7 @@ impl FacetedDocidsExtractor { if has_changed { // 1. Delete old facet values - let mut del = facet_fn_factory!(del); + let mut del = facet_fn!(del); extract_document_facets( inner.current(rtxn, index, context.db_fields_ids_map)?, @@ -191,12 +191,12 @@ impl FacetedDocidsExtractor { inner.current(rtxn, index, context.db_fields_ids_map)?, inner.external_document_id(), new_fields_ids_map.deref_mut(), - asc_desc_fields, &mut del, )?; } - let mut add = facet_fn_factory!(add); + // 2. Insert new facet values + let mut add = facet_fn!(add); extract_document_facets( inner.merged(rtxn, index, context.db_fields_ids_map)?, @@ -214,14 +214,13 @@ impl FacetedDocidsExtractor { inner.merged(rtxn, index, context.db_fields_ids_map)?, inner.external_document_id(), new_fields_ids_map.deref_mut(), - asc_desc_fields, &mut add, )?; } } } DocumentChange::Insertion(inner) => { - let mut add = facet_fn_factory!(add); + let mut add = facet_fn!(add); extract_document_facets( inner.inserted(), @@ -238,7 +237,6 @@ impl FacetedDocidsExtractor { inner.inserted(), inner.external_document_id(), new_fields_ids_map.deref_mut(), - asc_desc_fields, &mut add, )?; } diff --git a/crates/milli/src/update/new/extract/faceted/facet_document.rs b/crates/milli/src/update/new/extract/faceted/facet_document.rs index 30a5c462e..68bc98b64 100644 --- a/crates/milli/src/update/new/extract/faceted/facet_document.rs +++ b/crates/milli/src/update/new/extract/faceted/facet_document.rs @@ -108,7 +108,6 @@ pub fn extract_geo_document<'doc>( document: impl Document<'doc>, external_document_id: &str, field_id_map: &mut GlobalFieldsIdsMap, - asc_desc_fields: &HashSet, facet_fn: &mut impl FnMut(FieldId, Metadata, perm_json_p::Depth, &Value) -> Result<()>, ) -> Result<()> { if let Some(geo_value) = document.geo_field()? { From 6738a4f6ee4cad6fc341286e1fdd55898134df9e Mon Sep 17 00:00:00 2001 From: nnethercott Date: Mon, 26 May 2025 10:08:07 +0200 Subject: [PATCH 04/10] feat: mettre a jour the insta snapshots --- .../meilisearch/tests/documents/add_documents.rs | 16 ++++++++-------- .../update/new/extract/faceted/extract_facets.rs | 5 +---- .../update/new/extract/faceted/facet_document.rs | 1 - 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/crates/meilisearch/tests/documents/add_documents.rs b/crates/meilisearch/tests/documents/add_documents.rs index 6569bb9a5..39ad57750 100644 --- a/crates/meilisearch/tests/documents/add_documents.rs +++ b/crates/meilisearch/tests/documents/add_documents.rs @@ -2039,6 +2039,14 @@ async fn update_documents_with_geo_field() { @r###" { "hits": [ + { + "id": "4", + "_geo": { + "lat": "4", + "lng": "0" + }, + "_geoDistance": 667170 + }, { "id": "3", "_geo": { @@ -2048,14 +2056,6 @@ async fn update_documents_with_geo_field() { "doggo": "kefir", "_geoDistance": 555975 }, - { - "id": "4", - "_geo": { - "lat": "4", - "lng": "0" - }, - "_geoDistance": 667170 - }, { "id": "1" }, diff --git a/crates/milli/src/update/new/extract/faceted/extract_facets.rs b/crates/milli/src/update/new/extract/faceted/extract_facets.rs index de0edc164..3086d25e4 100644 --- a/crates/milli/src/update/new/extract/faceted/extract_facets.rs +++ b/crates/milli/src/update/new/extract/faceted/extract_facets.rs @@ -135,7 +135,6 @@ impl FacetedDocidsExtractor { extract_document_facets( inner.current(rtxn, index, context.db_fields_ids_map)?, - inner.external_document_id(), new_fields_ids_map.deref_mut(), filterable_attributes, sortable_fields, @@ -177,7 +176,6 @@ impl FacetedDocidsExtractor { extract_document_facets( inner.current(rtxn, index, context.db_fields_ids_map)?, - inner.external_document_id(), new_fields_ids_map.deref_mut(), filterable_attributes, sortable_fields, @@ -200,7 +198,6 @@ impl FacetedDocidsExtractor { extract_document_facets( inner.merged(rtxn, index, context.db_fields_ids_map)?, - inner.external_document_id(), new_fields_ids_map.deref_mut(), filterable_attributes, sortable_fields, @@ -224,7 +221,6 @@ impl FacetedDocidsExtractor { extract_document_facets( inner.inserted(), - inner.external_document_id(), new_fields_ids_map.deref_mut(), filterable_attributes, sortable_fields, @@ -232,6 +228,7 @@ impl FacetedDocidsExtractor { distinct_field, &mut add, )?; + if is_geo_enabled { extract_geo_document( inner.inserted(), diff --git a/crates/milli/src/update/new/extract/faceted/facet_document.rs b/crates/milli/src/update/new/extract/faceted/facet_document.rs index 68bc98b64..359c32e58 100644 --- a/crates/milli/src/update/new/extract/faceted/facet_document.rs +++ b/crates/milli/src/update/new/extract/faceted/facet_document.rs @@ -16,7 +16,6 @@ use crate::filterable_attributes_rules::match_faceted_field; #[allow(clippy::too_many_arguments)] pub fn extract_document_facets<'doc>( document: impl Document<'doc>, - external_document_id: &str, field_id_map: &mut GlobalFieldsIdsMap, filterable_attributes: &[FilterableAttributesRule], sortable_fields: &HashSet, From 18aed75d3b0221222c97bc445101c8f069918ac1 Mon Sep 17 00:00:00 2001 From: nnethercott Date: Mon, 26 May 2025 18:20:55 +0200 Subject: [PATCH 05/10] fix logic --- .../tests/documents/add_documents.rs | 16 +++---- .../new/extract/faceted/extract_facets.rs | 46 +++++++++---------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/crates/meilisearch/tests/documents/add_documents.rs b/crates/meilisearch/tests/documents/add_documents.rs index 39ad57750..6569bb9a5 100644 --- a/crates/meilisearch/tests/documents/add_documents.rs +++ b/crates/meilisearch/tests/documents/add_documents.rs @@ -2039,14 +2039,6 @@ async fn update_documents_with_geo_field() { @r###" { "hits": [ - { - "id": "4", - "_geo": { - "lat": "4", - "lng": "0" - }, - "_geoDistance": 667170 - }, { "id": "3", "_geo": { @@ -2056,6 +2048,14 @@ async fn update_documents_with_geo_field() { "doggo": "kefir", "_geoDistance": 555975 }, + { + "id": "4", + "_geo": { + "lat": "4", + "lng": "0" + }, + "_geoDistance": 667170 + }, { "id": "1" }, diff --git a/crates/milli/src/update/new/extract/faceted/extract_facets.rs b/crates/milli/src/update/new/extract/faceted/extract_facets.rs index 3086d25e4..861c67bbe 100644 --- a/crates/milli/src/update/new/extract/faceted/extract_facets.rs +++ b/crates/milli/src/update/new/extract/faceted/extract_facets.rs @@ -170,10 +170,10 @@ impl FacetedDocidsExtractor { let has_changed_for_geo_fields = inner.has_changed_for_geo_fields(rtxn, index, context.db_fields_ids_map)?; - if has_changed { - // 1. Delete old facet values - let mut del = facet_fn!(del); + // 1. Delete old facet values + let mut del = facet_fn!(del); + if has_changed { extract_document_facets( inner.current(rtxn, index, context.db_fields_ids_map)?, new_fields_ids_map.deref_mut(), @@ -183,19 +183,20 @@ impl FacetedDocidsExtractor { distinct_field, &mut del, )?; + } + if is_geo_enabled && has_changed_for_geo_fields { + extract_geo_document( + inner.current(rtxn, index, context.db_fields_ids_map)?, + inner.external_document_id(), + new_fields_ids_map.deref_mut(), + &mut del, + )?; + } - if is_geo_enabled && has_changed_for_geo_fields { - extract_geo_document( - inner.current(rtxn, index, context.db_fields_ids_map)?, - inner.external_document_id(), - new_fields_ids_map.deref_mut(), - &mut del, - )?; - } - - // 2. Insert new facet values - let mut add = facet_fn!(add); + // 2. Insert new facet values + let mut add = facet_fn!(add); + if has_changed { extract_document_facets( inner.merged(rtxn, index, context.db_fields_ids_map)?, new_fields_ids_map.deref_mut(), @@ -205,15 +206,14 @@ impl FacetedDocidsExtractor { distinct_field, &mut add, )?; - - if is_geo_enabled && has_changed_for_geo_fields { - extract_geo_document( - inner.merged(rtxn, index, context.db_fields_ids_map)?, - inner.external_document_id(), - new_fields_ids_map.deref_mut(), - &mut add, - )?; - } + } + if is_geo_enabled && has_changed_for_geo_fields { + extract_geo_document( + inner.merged(rtxn, index, context.db_fields_ids_map)?, + inner.external_document_id(), + new_fields_ids_map.deref_mut(), + &mut add, + )?; } } DocumentChange::Insertion(inner) => { From c9ec502ed9e37ca970f9a0954780003696452913 Mon Sep 17 00:00:00 2001 From: nnethercott Date: Mon, 26 May 2025 18:32:59 +0200 Subject: [PATCH 06/10] refactor for readability --- .../new/extract/faceted/extract_facets.rs | 31 +++++++------------ 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/crates/milli/src/update/new/extract/faceted/extract_facets.rs b/crates/milli/src/update/new/extract/faceted/extract_facets.rs index 861c67bbe..2640ac462 100644 --- a/crates/milli/src/update/new/extract/faceted/extract_facets.rs +++ b/crates/milli/src/update/new/extract/faceted/extract_facets.rs @@ -170,9 +170,7 @@ impl FacetedDocidsExtractor { let has_changed_for_geo_fields = inner.has_changed_for_geo_fields(rtxn, index, context.db_fields_ids_map)?; - // 1. Delete old facet values - let mut del = facet_fn!(del); - + // 1. Maybe update doc if has_changed { extract_document_facets( inner.current(rtxn, index, context.db_fields_ids_map)?, @@ -181,22 +179,9 @@ impl FacetedDocidsExtractor { sortable_fields, asc_desc_fields, distinct_field, - &mut del, + &mut facet_fn!(del), )?; - } - if is_geo_enabled && has_changed_for_geo_fields { - extract_geo_document( - inner.current(rtxn, index, context.db_fields_ids_map)?, - inner.external_document_id(), - new_fields_ids_map.deref_mut(), - &mut del, - )?; - } - // 2. Insert new facet values - let mut add = facet_fn!(add); - - if has_changed { extract_document_facets( inner.merged(rtxn, index, context.db_fields_ids_map)?, new_fields_ids_map.deref_mut(), @@ -204,15 +189,23 @@ impl FacetedDocidsExtractor { sortable_fields, asc_desc_fields, distinct_field, - &mut add, + &mut facet_fn!(add), )?; } + + // 2. Maybe update geo if is_geo_enabled && has_changed_for_geo_fields { + extract_geo_document( + inner.current(rtxn, index, context.db_fields_ids_map)?, + inner.external_document_id(), + new_fields_ids_map.deref_mut(), + &mut facet_fn!(del), + )?; extract_geo_document( inner.merged(rtxn, index, context.db_fields_ids_map)?, inner.external_document_id(), new_fields_ids_map.deref_mut(), - &mut add, + &mut facet_fn!(add), )?; } } From 9ad43b6841aa4765b7f31645631c69dce565d612 Mon Sep 17 00:00:00 2001 From: nnethercott Date: Mon, 26 May 2025 18:37:20 +0200 Subject: [PATCH 07/10] rename has_changed to has_changed_for_facets --- crates/milli/src/update/new/extract/faceted/extract_facets.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/milli/src/update/new/extract/faceted/extract_facets.rs b/crates/milli/src/update/new/extract/faceted/extract_facets.rs index 2640ac462..a1d9e6553 100644 --- a/crates/milli/src/update/new/extract/faceted/extract_facets.rs +++ b/crates/milli/src/update/new/extract/faceted/extract_facets.rs @@ -153,7 +153,7 @@ impl FacetedDocidsExtractor { } } DocumentChange::Update(inner) => { - let has_changed = inner.has_changed_for_fields( + let has_changed_for_facets = inner.has_changed_for_fields( &mut |field_name| { match_faceted_field( field_name, @@ -171,7 +171,7 @@ impl FacetedDocidsExtractor { inner.has_changed_for_geo_fields(rtxn, index, context.db_fields_ids_map)?; // 1. Maybe update doc - if has_changed { + if has_changed_for_facets { extract_document_facets( inner.current(rtxn, index, context.db_fields_ids_map)?, new_fields_ids_map.deref_mut(), From 44f812c36d958a3fd2477a943b649064e6fe8512 Mon Sep 17 00:00:00 2001 From: Nate Nethercott <53127799+nnethercott@users.noreply.github.com> Date: Wed, 28 May 2025 15:38:12 +0200 Subject: [PATCH 08/10] Update crates/milli/src/update/new/extract/faceted/extract_facets.rs Co-authored-by: Many the fish --- crates/milli/src/update/new/extract/faceted/extract_facets.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/milli/src/update/new/extract/faceted/extract_facets.rs b/crates/milli/src/update/new/extract/faceted/extract_facets.rs index a1d9e6553..9e5120bba 100644 --- a/crates/milli/src/update/new/extract/faceted/extract_facets.rs +++ b/crates/milli/src/update/new/extract/faceted/extract_facets.rs @@ -194,7 +194,7 @@ impl FacetedDocidsExtractor { } // 2. Maybe update geo - if is_geo_enabled && has_changed_for_geo_fields { + if is_geo_enabled && inner.has_changed_for_geo_fields(rtxn, index, context.db_fields_ids_map)? { extract_geo_document( inner.current(rtxn, index, context.db_fields_ids_map)?, inner.external_document_id(), From b06cc1e0a2bba4f89fab8e80fd6e7e08a5670651 Mon Sep 17 00:00:00 2001 From: Nate Nethercott <53127799+nnethercott@users.noreply.github.com> Date: Wed, 28 May 2025 15:38:23 +0200 Subject: [PATCH 09/10] Update crates/milli/src/update/new/extract/faceted/extract_facets.rs Co-authored-by: Many the fish --- crates/milli/src/update/new/extract/faceted/extract_facets.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/milli/src/update/new/extract/faceted/extract_facets.rs b/crates/milli/src/update/new/extract/faceted/extract_facets.rs index 9e5120bba..2160c16eb 100644 --- a/crates/milli/src/update/new/extract/faceted/extract_facets.rs +++ b/crates/milli/src/update/new/extract/faceted/extract_facets.rs @@ -91,7 +91,8 @@ impl FacetedDocidsExtractor { let mut del_add_facet_value = DelAddFacetValue::new(&context.doc_alloc); let docid = document_change.docid(); - // Macro expanding to an insertion/deletion facet fn + // Macro expanding to an insertion/deletion facet fn, + // using a macro avoid to borrow the parameters as mutable in both closures at the same time by postponing their creation macro_rules! facet_fn { (del) => { |fid: FieldId, meta: Metadata, depth: perm_json_p::Depth, value: &Value| { From 1811168b965fbfe695bbd5bfec4904d6bc96aab6 Mon Sep 17 00:00:00 2001 From: nnethercott Date: Wed, 28 May 2025 15:45:13 +0200 Subject: [PATCH 10/10] remove duplicated check on geo field changes --- .../src/update/new/extract/faceted/extract_facets.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/milli/src/update/new/extract/faceted/extract_facets.rs b/crates/milli/src/update/new/extract/faceted/extract_facets.rs index 2160c16eb..517ef3f2d 100644 --- a/crates/milli/src/update/new/extract/faceted/extract_facets.rs +++ b/crates/milli/src/update/new/extract/faceted/extract_facets.rs @@ -91,8 +91,8 @@ impl FacetedDocidsExtractor { let mut del_add_facet_value = DelAddFacetValue::new(&context.doc_alloc); let docid = document_change.docid(); - // Macro expanding to an insertion/deletion facet fn, - // using a macro avoid to borrow the parameters as mutable in both closures at the same time by postponing their creation + // Using a macro avoid borrowing the parameters as mutable in both closures at + // the same time by postponing their creation macro_rules! facet_fn { (del) => { |fid: FieldId, meta: Metadata, depth: perm_json_p::Depth, value: &Value| { @@ -168,8 +168,6 @@ impl FacetedDocidsExtractor { index, context.db_fields_ids_map, )?; - let has_changed_for_geo_fields = - inner.has_changed_for_geo_fields(rtxn, index, context.db_fields_ids_map)?; // 1. Maybe update doc if has_changed_for_facets { @@ -195,7 +193,9 @@ impl FacetedDocidsExtractor { } // 2. Maybe update geo - if is_geo_enabled && inner.has_changed_for_geo_fields(rtxn, index, context.db_fields_ids_map)? { + if is_geo_enabled + && inner.has_changed_for_geo_fields(rtxn, index, context.db_fields_ids_map)? + { extract_geo_document( inner.current(rtxn, index, context.db_fields_ids_map)?, inner.external_document_id(),