From 7aa6cc9b04ab78ca5374faf2e1118d7fa6b269da Mon Sep 17 00:00:00 2001 From: Kerollmops Date: Thu, 22 Jul 2021 17:11:17 +0200 Subject: [PATCH] Do not insert fields in the map when changing the settings --- milli/src/index.rs | 50 ++++---------------- milli/src/search/criteria/asc_desc.rs | 32 +++++++------ milli/src/search/facet/facet_distribution.rs | 13 ++--- milli/src/search/mod.rs | 15 +++--- milli/src/update/delete_documents.rs | 14 +++--- milli/src/update/settings.rs | 18 ------- 6 files changed, 46 insertions(+), 96 deletions(-) diff --git a/milli/src/index.rs b/milli/src/index.rs index f26643de7..63da6b1e8 100644 --- a/milli/src/index.rs +++ b/milli/src/index.rs @@ -8,7 +8,7 @@ use heed::types::*; use heed::{Database, PolyDatabase, RoTxn, RwTxn}; use roaring::RoaringBitmap; -use crate::error::{FieldIdMapMissingEntry, InternalError, UserError}; +use crate::error::{InternalError, UserError}; use crate::fields_ids_map::FieldsIdsMap; use crate::heed_codec::facet::{ FacetLevelValueF64Codec, FacetStringLevelZeroCodec, FacetStringLevelZeroValueCodec, @@ -353,15 +353,8 @@ impl Index { let fields_ids_map = self.fields_ids_map(rtxn)?; let mut fields_ids = Vec::new(); for name in fields.into_iter() { - match fields_ids_map.id(name) { - Some(field_id) => fields_ids.push(field_id), - None => { - return Err(FieldIdMapMissingEntry::FieldName { - field_name: name.to_string(), - process: "Index::displayed_fields_ids", - } - .into()) - } + if let Some(field_id) = fields_ids_map.id(name) { + fields_ids.push(field_id); } } Ok(Some(fields_ids)) @@ -403,15 +396,8 @@ impl Index { let fields_ids_map = self.fields_ids_map(rtxn)?; let mut fields_ids = Vec::new(); for name in fields { - match fields_ids_map.id(name) { - Some(field_id) => fields_ids.push(field_id), - None => { - return Err(FieldIdMapMissingEntry::FieldName { - field_name: name.to_string(), - process: "Index::searchable_fields_ids", - } - .into()) - } + if let Some(field_id) = fields_ids_map.id(name) { + fields_ids.push(field_id); } } Ok(Some(fields_ids)) @@ -451,17 +437,8 @@ impl Index { let mut fields_ids = HashSet::new(); for name in fields { - match fields_ids_map.id(&name) { - Some(field_id) => { - fields_ids.insert(field_id); - } - None => { - return Err(FieldIdMapMissingEntry::FieldName { - field_name: name, - process: "Index::filterable_fields_ids", - } - .into()) - } + if let Some(field_id) = fields_ids_map.id(&name) { + fields_ids.insert(field_id); } } @@ -498,17 +475,8 @@ impl Index { let mut fields_ids = HashSet::new(); for name in fields.into_iter() { - match fields_ids_map.id(&name) { - Some(field_id) => { - fields_ids.insert(field_id); - } - None => { - return Err(FieldIdMapMissingEntry::FieldName { - field_name: name, - process: "Index::faceted_fields_ids", - } - .into()) - } + if let Some(field_id) = fields_ids_map.id(&name) { + fields_ids.insert(field_id); } } diff --git a/milli/src/search/criteria/asc_desc.rs b/milli/src/search/criteria/asc_desc.rs index 99d63c90d..4a664d042 100644 --- a/milli/src/search/criteria/asc_desc.rs +++ b/milli/src/search/criteria/asc_desc.rs @@ -6,7 +6,6 @@ use ordered_float::OrderedFloat; use roaring::RoaringBitmap; use super::{Criterion, CriterionParameters, CriterionResult}; -use crate::error::FieldIdMapMissingEntry; use crate::search::criteria::{resolve_query_tree, CriteriaBuilder}; use crate::search::facet::FacetNumberIter; use crate::search::query_tree::Operation; @@ -20,7 +19,7 @@ pub struct AscDesc<'t> { index: &'t Index, rtxn: &'t heed::RoTxn<'t>, field_name: String, - field_id: FieldId, + field_id: Option, ascending: bool, query_tree: Option, candidates: Box> + 't>, @@ -57,11 +56,11 @@ impl<'t> AscDesc<'t> { ascending: bool, ) -> Result { let fields_ids_map = index.fields_ids_map(rtxn)?; - let field_id = - fields_ids_map.id(&field_name).ok_or_else(|| FieldIdMapMissingEntry::FieldName { - field_name: field_name.clone(), - process: "AscDesc::new", - })?; + let field_id = fields_ids_map.id(&field_name); + let faceted_candidates = match field_id { + Some(field_id) => index.number_faceted_documents_ids(rtxn, field_id)?, + None => RoaringBitmap::default(), + }; Ok(AscDesc { index, @@ -72,7 +71,7 @@ impl<'t> AscDesc<'t> { query_tree: None, candidates: Box::new(std::iter::empty()), allowed_candidates: RoaringBitmap::new(), - faceted_candidates: index.number_faceted_documents_ids(rtxn, field_id)?, + faceted_candidates, bucket_candidates: RoaringBitmap::new(), parent, }) @@ -132,13 +131,16 @@ impl<'t> Criterion for AscDesc<'t> { } self.allowed_candidates = &candidates - params.excluded_candidates; - self.candidates = facet_ordered( - self.index, - self.rtxn, - self.field_id, - self.ascending, - candidates & &self.faceted_candidates, - )?; + self.candidates = match self.field_id { + Some(field_id) => facet_ordered( + self.index, + self.rtxn, + field_id, + self.ascending, + candidates & &self.faceted_candidates, + )?, + None => Box::new(std::iter::empty()), + }; } None => return Ok(None), }, diff --git a/milli/src/search/facet/facet_distribution.rs b/milli/src/search/facet/facet_distribution.rs index 94f875dfc..bfbea76c3 100644 --- a/milli/src/search/facet/facet_distribution.rs +++ b/milli/src/search/facet/facet_distribution.rs @@ -5,7 +5,7 @@ use std::{cmp, fmt, mem}; use heed::types::ByteSlice; use roaring::RoaringBitmap; -use crate::error::{FieldIdMapMissingEntry, UserError}; +use crate::error::UserError; use crate::facet::FacetType; use crate::heed_codec::facet::{ FacetStringLevelZeroCodec, FieldDocIdFacetF64Codec, FieldDocIdFacetStringCodec, @@ -277,13 +277,10 @@ impl<'a> FacetDistribution<'a> { let mut distribution = BTreeMap::new(); for name in fields { - let fid = - fields_ids_map.id(&name).ok_or_else(|| FieldIdMapMissingEntry::FieldName { - field_name: name.clone(), - process: "FacetDistribution::execute", - })?; - let values = self.facet_values(fid)?; - distribution.insert(name, values); + if let Some(fid) = fields_ids_map.id(&name) { + let values = self.facet_values(fid)?; + distribution.insert(name, values); + } } Ok(distribution) diff --git a/milli/src/search/mod.rs b/milli/src/search/mod.rs index 574459547..871f464ef 100644 --- a/milli/src/search/mod.rs +++ b/milli/src/search/mod.rs @@ -18,7 +18,6 @@ pub(crate) use self::facet::ParserRule; pub use self::facet::{FacetDistribution, FacetNumberIter, FilterCondition, Operator}; pub use self::matching_words::MatchingWords; use self::query_tree::QueryTreeBuilder; -use crate::error::FieldIdMapMissingEntry; use crate::search::criteria::r#final::{Final, FinalResult}; use crate::{DocumentId, Index, Result}; @@ -142,13 +141,13 @@ impl<'a> Search<'a> { None => self.perform_sort(NoopDistinct, matching_words, criteria), Some(name) => { let field_ids_map = self.index.fields_ids_map(self.rtxn)?; - let id = - field_ids_map.id(name).ok_or_else(|| FieldIdMapMissingEntry::FieldName { - field_name: name.to_string(), - process: "distinct attribute", - })?; - let distinct = FacetDistinct::new(id, self.index, self.rtxn); - self.perform_sort(distinct, matching_words, criteria) + match field_ids_map.id(name) { + Some(fid) => { + let distinct = FacetDistinct::new(fid, self.index, self.rtxn); + self.perform_sort(distinct, matching_words, criteria) + } + None => Ok(SearchResult::default()), + } } } } diff --git a/milli/src/update/delete_documents.rs b/milli/src/update/delete_documents.rs index bcb7d7580..bd56688f6 100644 --- a/milli/src/update/delete_documents.rs +++ b/milli/src/update/delete_documents.rs @@ -8,7 +8,7 @@ use roaring::RoaringBitmap; use serde_json::Value; use super::ClearDocuments; -use crate::error::{FieldIdMapMissingEntry, InternalError, UserError}; +use crate::error::{InternalError, UserError}; use crate::heed_codec::facet::FacetStringLevelZeroValueCodec; use crate::heed_codec::CboRoaringBitmapCodec; use crate::index::{db_name, main_key}; @@ -82,11 +82,13 @@ impl<'t, 'u, 'i> DeleteDocuments<'t, 'u, 'i> { key: Some(main_key::PRIMARY_KEY_KEY), } })?; - let id_field = - fields_ids_map.id(primary_key).ok_or_else(|| FieldIdMapMissingEntry::FieldName { - field_name: primary_key.to_string(), - process: "DeleteDocuments::execute", - })?; + + // If we can't find the id of the primary key it means that the database + // is empty and it should be safe to return that we deleted 0 documents. + let id_field = match fields_ids_map.id(primary_key) { + Some(field) => field, + None => return Ok(0), + }; let Index { env: _env, diff --git a/milli/src/update/settings.rs b/milli/src/update/settings.rs index a78cc9da8..743483613 100644 --- a/milli/src/update/settings.rs +++ b/milli/src/update/settings.rs @@ -235,15 +235,9 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { fn update_displayed(&mut self) -> Result { match self.displayed_fields { Setting::Set(ref fields) => { - let mut fields_ids_map = self.index.fields_ids_map(self.wtxn)?; // fields are deduplicated, only the first occurrence is taken into account let names: Vec<_> = fields.iter().unique().map(String::as_str).collect(); - - for name in names.iter() { - fields_ids_map.insert(name).ok_or(UserError::AttributeLimitReached)?; - } self.index.put_displayed_fields(self.wtxn, &names)?; - self.index.put_fields_ids_map(self.wtxn, &fields_ids_map)?; } Setting::Reset => { self.index.delete_displayed_fields(self.wtxn)?; @@ -256,11 +250,7 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { fn update_distinct_field(&mut self) -> Result { match self.distinct_field { Setting::Set(ref attr) => { - let mut fields_ids_map = self.index.fields_ids_map(self.wtxn)?; - fields_ids_map.insert(attr).ok_or(UserError::AttributeLimitReached)?; - self.index.put_distinct_field(self.wtxn, &attr)?; - self.index.put_fields_ids_map(self.wtxn, &fields_ids_map)?; } Setting::Reset => { self.index.delete_distinct_field(self.wtxn)?; @@ -388,14 +378,11 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { fn update_filterable(&mut self) -> Result<()> { match self.filterable_fields { Setting::Set(ref fields) => { - let mut fields_ids_map = self.index.fields_ids_map(self.wtxn)?; let mut new_facets = HashSet::new(); for name in fields { - fields_ids_map.insert(name).ok_or(UserError::AttributeLimitReached)?; new_facets.insert(name.clone()); } self.index.put_filterable_fields(self.wtxn, &new_facets)?; - self.index.put_fields_ids_map(self.wtxn, &fields_ids_map)?; } Setting::Reset => { self.index.delete_filterable_fields(self.wtxn)?; @@ -408,17 +395,12 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { fn update_criteria(&mut self) -> Result<()> { match self.criteria { Setting::Set(ref fields) => { - let mut fields_ids_map = self.index.fields_ids_map(self.wtxn)?; let mut new_criteria = Vec::new(); for name in fields { let criterion: Criterion = name.parse()?; - if let Some(name) = criterion.field_name() { - fields_ids_map.insert(name).ok_or(UserError::AttributeLimitReached)?; - } new_criteria.push(criterion); } self.index.put_criteria(self.wtxn, &new_criteria)?; - self.index.put_fields_ids_map(self.wtxn, &fields_ids_map)?; } Setting::Reset => { self.index.delete_criteria(self.wtxn)?;