From 8ec0c322eaf7c329d946a7e2fac12c2010677282 Mon Sep 17 00:00:00 2001 From: ManyTheFish Date: Thu, 6 Mar 2025 11:42:53 +0100 Subject: [PATCH] Apply PR requests related to Refactor the FieldIdMapWithMetadata --- crates/milli/src/fields_ids_map/global.rs | 11 +++++-- crates/milli/src/fields_ids_map/metadata.rs | 29 +++++++++++++++---- .../src/search/facet/facet_distribution.rs | 22 +++++++++----- .../src/update/new/indexer/post_processing.rs | 6 ++-- 4 files changed, 49 insertions(+), 19 deletions(-) diff --git a/crates/milli/src/fields_ids_map/global.rs b/crates/milli/src/fields_ids_map/global.rs index e5f1212df..235d509e9 100644 --- a/crates/milli/src/fields_ids_map/global.rs +++ b/crates/milli/src/fields_ids_map/global.rs @@ -107,8 +107,15 @@ impl<'indexing> GlobalFieldsIdsMap<'indexing> { } /// Get the metadata of a field based on its id. - pub fn metadata(&self, id: FieldId) -> Option { - self.local.metadata(id).or_else(|| self.global.read().unwrap().metadata(id)) + pub fn metadata(&mut self, id: FieldId) -> Option { + if self.local.metadata(id).is_none() { + let global = self.global.read().unwrap(); + + let (name, metadata) = global.name_with_metadata(id)?; + self.local.insert(name, id, metadata); + } + + self.local.metadata(id) } } diff --git a/crates/milli/src/fields_ids_map/metadata.rs b/crates/milli/src/fields_ids_map/metadata.rs index 5636256eb..7f81e6b79 100644 --- a/crates/milli/src/fields_ids_map/metadata.rs +++ b/crates/milli/src/fields_ids_map/metadata.rs @@ -22,7 +22,7 @@ pub struct Metadata { pub distinct: bool, /// The field has been defined as asc/desc in the ranking rules. pub asc_desc: bool, - /// The field is a geo field. + /// The field is a geo field (`_geo`, `_geo.lat`, `_geo.lng`). pub geo: bool, /// The id of the localized attributes rule if the field is localized. pub localized_attributes_rule_id: Option, @@ -215,9 +215,8 @@ pub struct MetadataBuilder { impl MetadataBuilder { pub fn from_index(index: &Index, rtxn: &RoTxn) -> Result { let searchable_attributes = match index.user_defined_searchable_fields(rtxn)? { - Some(fields) if fields.contains(&"*") => None, - None => None, Some(fields) => Some(fields.into_iter().map(|s| s.to_string()).collect()), + None => None, }; let filterable_attributes = index.filterable_attributes_rules(rtxn)?; let sortable_attributes = index.sortable_fields(rtxn)?; @@ -225,14 +224,14 @@ impl MetadataBuilder { let distinct_attribute = index.distinct_field(rtxn)?.map(|s| s.to_string()); let asc_desc_attributes = index.asc_desc_fields(rtxn)?; - Ok(Self { + Ok(Self::_new( searchable_attributes, filterable_attributes, sortable_attributes, localized_attributes, distinct_attribute, asc_desc_attributes, - }) + )) } #[cfg(test)] @@ -246,11 +245,29 @@ impl MetadataBuilder { localized_attributes: Option>, distinct_attribute: Option, asc_desc_attributes: HashSet, + ) -> Self { + Self::_new( + searchable_attributes, + filterable_attributes, + sortable_attributes, + localized_attributes, + distinct_attribute, + asc_desc_attributes, + ) + } + + fn _new( + searchable_attributes: Option>, + filterable_attributes: Vec, + sortable_attributes: HashSet, + localized_attributes: Option>, + distinct_attribute: Option, + asc_desc_attributes: HashSet, ) -> Self { let searchable_attributes = match searchable_attributes { Some(fields) if fields.iter().any(|f| f == "*") => None, - None => None, Some(fields) => Some(fields), + None => None, }; Self { diff --git a/crates/milli/src/search/facet/facet_distribution.rs b/crates/milli/src/search/facet/facet_distribution.rs index beb5d2568..5c41a0424 100644 --- a/crates/milli/src/search/facet/facet_distribution.rs +++ b/crates/milli/src/search/facet/facet_distribution.rs @@ -294,11 +294,7 @@ impl<'a> FacetDistribution<'a> { return Ok(Default::default()); }; - let fields_ids_map = self.index.fields_ids_map(self.rtxn)?; - let fields_ids_map = FieldIdMapWithMetadata::new( - fields_ids_map, - MetadataBuilder::from_index(self.index, self.rtxn)?, - ); + let fields_ids_map = self.index.fields_ids_map_with_metadata(self.rtxn)?; let filterable_attributes_rules = self.index.filterable_attributes_rules(self.rtxn)?; self.check_faceted_fields(&fields_ids_map, &filterable_attributes_rules)?; @@ -365,12 +361,17 @@ impl<'a> FacetDistribution<'a> { metadata: &Metadata, filterable_attributes_rules: &[FilterableAttributesRule], ) -> bool { + // If the field is not filterable, we don't want to compute the facet distribution. + if !metadata.filterable_attributes_features(filterable_attributes_rules).is_filterable() { + return false; + } + match &self.facets { Some(facets) => { // The list of facets provided by the user is a legacy pattern ("dog.age" must be selected with "dog"). facets.keys().any(|key| match_field_legacy(key, name) == PatternMatch::Match) } - None => metadata.is_faceted(filterable_attributes_rules), + None => true, } } @@ -385,7 +386,9 @@ impl<'a> FacetDistribution<'a> { for field in facets.keys() { let is_valid_faceted_field = fields_ids_map.id_with_metadata(field).map_or(false, |(_, metadata)| { - metadata.is_faceted(filterable_attributes_rules) + metadata + .filterable_attributes_features(filterable_attributes_rules) + .is_filterable() }); if !is_valid_faceted_field { invalid_facets.insert(field.to_string()); @@ -397,7 +400,10 @@ impl<'a> FacetDistribution<'a> { let valid_facets_name = fields_ids_map .iter() .filter_map(|(_, name, metadata)| { - if metadata.is_faceted(filterable_attributes_rules) { + if metadata + .filterable_attributes_features(filterable_attributes_rules) + .is_filterable() + { Some(name.to_string()) } else { None diff --git a/crates/milli/src/update/new/indexer/post_processing.rs b/crates/milli/src/update/new/indexer/post_processing.rs index 4ea749a85..2a01fccf3 100644 --- a/crates/milli/src/update/new/indexer/post_processing.rs +++ b/crates/milli/src/update/new/indexer/post_processing.rs @@ -25,7 +25,7 @@ use crate::{GlobalFieldsIdsMap, Index, Result}; pub(super) fn post_process( indexing_context: IndexingContext, wtxn: &mut RwTxn<'_>, - global_fields_ids_map: GlobalFieldsIdsMap<'_>, + mut global_fields_ids_map: GlobalFieldsIdsMap<'_>, facet_field_ids_delta: FacetFieldIdsDelta, ) -> Result<()> where @@ -33,7 +33,7 @@ where { let index = indexing_context.index; indexing_context.progress.update_progress(IndexingStep::PostProcessingFacets); - compute_facet_level_database(index, wtxn, facet_field_ids_delta, &global_fields_ids_map)?; + compute_facet_level_database(index, wtxn, facet_field_ids_delta, &mut global_fields_ids_map)?; compute_facet_search_database(index, wtxn, global_fields_ids_map)?; indexing_context.progress.update_progress(IndexingStep::PostProcessingWords); if let Some(prefix_delta) = compute_word_fst(index, wtxn)? { @@ -170,7 +170,7 @@ fn compute_facet_level_database( index: &Index, wtxn: &mut RwTxn, mut facet_field_ids_delta: FacetFieldIdsDelta, - global_fields_ids_map: &GlobalFieldsIdsMap, + global_fields_ids_map: &mut GlobalFieldsIdsMap, ) -> Result<()> { let rtxn = index.read_txn()?; let filterable_attributes_rules = index.filterable_attributes_rules(&rtxn)?;