From aeaac743ff08d9599084c0375f694a842b698f03 Mon Sep 17 00:00:00 2001 From: Kerollmops Date: Wed, 23 Jun 2021 11:33:30 +0200 Subject: [PATCH 1/3] Replace an if let some by a match --- milli/src/search/facet/facet_distribution.rs | 67 ++++++++++---------- 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/milli/src/search/facet/facet_distribution.rs b/milli/src/search/facet/facet_distribution.rs index 0a2036494..1a66acbfa 100644 --- a/milli/src/search/facet/facet_distribution.rs +++ b/milli/src/search/facet/facet_distribution.rs @@ -174,40 +174,41 @@ impl<'a> FacetDistribution<'a> { fn facet_values(&self, field_id: FieldId) -> heed::Result> { use FacetType::{Number, String}; - if let Some(candidates) = self.candidates.as_ref() { - // Classic search, candidates were specified, we must return facet values only related - // to those candidates. We also enter here for facet strings for performance reasons. - let mut distribution = BTreeMap::new(); - if candidates.len() <= CANDIDATES_THRESHOLD { - self.facet_distribution_from_documents( - field_id, - Number, - candidates, - &mut distribution, - )?; - self.facet_distribution_from_documents( - field_id, - String, - candidates, - &mut distribution, - )?; - } else { - self.facet_numbers_distribution_from_facet_levels( - field_id, - candidates, - &mut distribution, - )?; - self.facet_distribution_from_documents( - field_id, - String, - candidates, - &mut distribution, - )?; - } + match self.candidates { + Some(ref candidates) => { + // Classic search, candidates were specified, we must return facet values only related + // to those candidates. We also enter here for facet strings for performance reasons. + let mut distribution = BTreeMap::new(); + if candidates.len() <= CANDIDATES_THRESHOLD { + self.facet_distribution_from_documents( + field_id, + Number, + candidates, + &mut distribution, + )?; + self.facet_distribution_from_documents( + field_id, + String, + candidates, + &mut distribution, + )?; + } else { + self.facet_numbers_distribution_from_facet_levels( + field_id, + candidates, + &mut distribution, + )?; + self.facet_distribution_from_documents( + field_id, + String, + candidates, + &mut distribution, + )?; + } - Ok(distribution) - } else { - self.facet_values_from_raw_facet_database(field_id) + Ok(distribution) + } + None => self.facet_values_from_raw_facet_database(field_id), } } From 2364777838c03cd393d3b407efbef6ecb19d4684 Mon Sep 17 00:00:00 2001 From: Kerollmops Date: Wed, 23 Jun 2021 11:50:49 +0200 Subject: [PATCH 2/3] Return an error for when a field distribution cannot be done --- milli/src/search/facet/facet_distribution.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/milli/src/search/facet/facet_distribution.rs b/milli/src/search/facet/facet_distribution.rs index 1a66acbfa..4d077b8f1 100644 --- a/milli/src/search/facet/facet_distribution.rs +++ b/milli/src/search/facet/facet_distribution.rs @@ -215,9 +215,20 @@ impl<'a> FacetDistribution<'a> { pub fn execute(&self) -> Result>> { let fields_ids_map = self.index.fields_ids_map(self.rtxn)?; let filterable_fields = self.index.filterable_fields(self.rtxn)?; + let fields = match self.facets { + Some(ref facets) => { + let invalid_fields: HashSet<_> = facets.difference(&filterable_fields).collect(); + if !invalid_fields.is_empty() { + todo!("return an error specifying that these fields are not filterable"); + } else { + facets.clone() + } + } + None => filterable_fields, + }; let mut distribution = BTreeMap::new(); - for name in filterable_fields { + for name in fields { let fid = fields_ids_map.id(&name).ok_or_else(|| FieldIdMapMissingEntry::FieldName { field_name: name.clone(), From a6218a20ae3eedbdd0927e3dc850cf6f960e6b2c Mon Sep 17 00:00:00 2001 From: Kerollmops Date: Wed, 23 Jun 2021 13:56:13 +0200 Subject: [PATCH 3/3] Introduce a new InvalidFacetsDistribution user error --- milli/src/error.rs | 15 +++++++++++++-- milli/src/search/facet/facet_distribution.rs | 7 +++++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/milli/src/error.rs b/milli/src/error.rs index d927407f0..713935869 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::convert::Infallible; use std::error::Error as StdError; use std::{fmt, io, str}; @@ -51,13 +52,14 @@ pub enum FieldIdMapMissingEntry { pub enum UserError { AttributeLimitReached, Csv(csv::Error), - MaxDatabaseSizeReached, DocumentLimitReached, - InvalidFilter(pest::error::Error), InvalidCriterionName { name: String }, InvalidDocumentId { document_id: Value }, + InvalidFacetsDistribution { invalid_facets_name: HashSet }, + InvalidFilter(pest::error::Error), InvalidFilterAttribute(pest::error::Error), InvalidStoreFile, + MaxDatabaseSizeReached, MissingDocumentId { document: Object }, MissingPrimaryKey, NoSpaceLeftOnDevice, @@ -202,6 +204,15 @@ impl fmt::Display for UserError { Self::AttributeLimitReached => f.write_str("maximum number of attributes reached"), Self::Csv(error) => error.fmt(f), Self::DocumentLimitReached => f.write_str("maximum number of documents reached"), + Self::InvalidFacetsDistribution { invalid_facets_name } => { + let name_list = + invalid_facets_name.iter().map(AsRef::as_ref).collect::>().join(", "); + write!( + f, + "invalid facet distribution, the fields {} are not set as filterable", + name_list + ) + } Self::InvalidFilter(error) => error.fmt(f), Self::InvalidCriterionName { name } => write!(f, "invalid criterion {}", name), Self::InvalidDocumentId { document_id } => { diff --git a/milli/src/search/facet/facet_distribution.rs b/milli/src/search/facet/facet_distribution.rs index 4d077b8f1..71816cf5d 100644 --- a/milli/src/search/facet/facet_distribution.rs +++ b/milli/src/search/facet/facet_distribution.rs @@ -6,7 +6,7 @@ use heed::types::{ByteSlice, Unit}; use heed::{BytesDecode, Database}; use roaring::RoaringBitmap; -use crate::error::FieldIdMapMissingEntry; +use crate::error::{FieldIdMapMissingEntry, UserError}; use crate::facet::FacetType; use crate::heed_codec::facet::FacetValueStringCodec; use crate::search::facet::{FacetIter, FacetRange}; @@ -219,7 +219,10 @@ impl<'a> FacetDistribution<'a> { Some(ref facets) => { let invalid_fields: HashSet<_> = facets.difference(&filterable_fields).collect(); if !invalid_fields.is_empty() { - todo!("return an error specifying that these fields are not filterable"); + return Err(UserError::InvalidFacetsDistribution { + invalid_facets_name: invalid_fields.into_iter().cloned().collect(), + } + .into()); } else { facets.clone() }