From 922f9fd4d5cd768bf585dd4c297a110849b52fb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9mentine=20Urquizar?= Date: Fri, 20 Aug 2021 18:09:17 +0200 Subject: [PATCH] Revert "Sort at query time" --- benchmarks/benches/search_songs.rs | 8 +- http-ui/src/main.rs | 4 +- milli/Cargo.toml | 1 + milli/src/criterion.rs | 69 ++---- milli/src/error.rs | 10 - milli/src/index.rs | 36 +-- milli/src/lib.rs | 2 +- milli/src/search/criteria/asc_desc.rs | 102 ++------ milli/src/search/criteria/mod.rs | 26 +- milli/src/search/facet/facet_string.rs | 328 ++++--------------------- milli/src/search/mod.rs | 34 +-- milli/src/update/settings.rs | 30 +-- milli/tests/assets/test_set.ndjson | 34 +-- milli/tests/search/distinct.rs | 2 +- milli/tests/search/filters.rs | 2 +- milli/tests/search/mod.rs | 23 +- milli/tests/search/query_criteria.rs | 138 ++--------- 17 files changed, 148 insertions(+), 701 deletions(-) diff --git a/benchmarks/benches/search_songs.rs b/benchmarks/benches/search_songs.rs index 6b11799ec..726040692 100644 --- a/benchmarks/benches/search_songs.rs +++ b/benchmarks/benches/search_songs.rs @@ -52,9 +52,9 @@ fn bench_songs(c: &mut criterion::Criterion) { milli::default_criteria().iter().map(|criteria| criteria.to_string()).collect(); let default_criterion = default_criterion.iter().map(|s| s.as_str()); let asc_default: Vec<&str> = - std::iter::once("released-timestamp:asc").chain(default_criterion.clone()).collect(); + std::iter::once("asc(released-timestamp)").chain(default_criterion.clone()).collect(); let desc_default: Vec<&str> = - std::iter::once("released-timestamp:desc").chain(default_criterion.clone()).collect(); + std::iter::once("desc(released-timestamp)").chain(default_criterion.clone()).collect(); let basic_with_quote: Vec = BASE_CONF .queries @@ -118,12 +118,12 @@ fn bench_songs(c: &mut criterion::Criterion) { }, utils::Conf { group_name: "asc", - criterion: Some(&["released-timestamp:desc"]), + criterion: Some(&["asc(released-timestamp)"]), ..BASE_CONF }, utils::Conf { group_name: "desc", - criterion: Some(&["released-timestamp:desc"]), + criterion: Some(&["desc(released-timestamp)"]), ..BASE_CONF }, diff --git a/http-ui/src/main.rs b/http-ui/src/main.rs index b34418465..ee32882c0 100644 --- a/http-ui/src/main.rs +++ b/http-ui/src/main.rs @@ -1030,7 +1030,7 @@ mod tests { displayed_attributes: Setting::Set(vec!["name".to_string()]), searchable_attributes: Setting::Set(vec!["age".to_string()]), filterable_attributes: Setting::Set(hashset! { "age".to_string() }), - criteria: Setting::Set(vec!["age:asc".to_string()]), + criteria: Setting::Set(vec!["asc(age)".to_string()]), stop_words: Setting::Set(btreeset! { "and".to_string() }), synonyms: Setting::Set(hashmap! { "alex".to_string() => vec!["alexey".to_string()] }), }; @@ -1058,7 +1058,7 @@ mod tests { Token::Str("criteria"), Token::Some, Token::Seq { len: Some(1) }, - Token::Str("age:asc"), + Token::Str("asc(age)"), Token::SeqEnd, Token::Str("stopWords"), Token::Some, diff --git a/milli/Cargo.toml b/milli/Cargo.toml index ae9298944..e860454d6 100644 --- a/milli/Cargo.toml +++ b/milli/Cargo.toml @@ -25,6 +25,7 @@ obkv = "0.2.0" once_cell = "1.5.2" ordered-float = "2.1.1" rayon = "1.5.0" +regex = "1.4.3" roaring = "0.6.6" serde = { version = "1.0.123", features = ["derive"] } serde_json = { version = "1.0.62", features = ["preserve_order"] } diff --git a/milli/src/criterion.rs b/milli/src/criterion.rs index 47eb7c7dc..cc1fca01f 100644 --- a/milli/src/criterion.rs +++ b/milli/src/criterion.rs @@ -1,10 +1,15 @@ use std::fmt; use std::str::FromStr; +use once_cell::sync::Lazy; +use regex::Regex; use serde::{Deserialize, Serialize}; use crate::error::{Error, UserError}; +static ASC_DESC_REGEX: Lazy = + Lazy::new(|| Regex::new(r#"(asc|desc)\(([\w_-]+)\)"#).unwrap()); + #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] pub enum Criterion { /// Sorted by decreasing number of matched query terms. @@ -12,13 +17,10 @@ pub enum Criterion { Words, /// Sorted by increasing number of typos. Typo, - /// Dynamically sort at query time the documents. None, one or multiple Asc/Desc sortable - /// attributes can be used in place of this criterion at query time. - Sort, /// Sorted by increasing distance between matched query terms. Proximity, /// Documents with quey words contained in more important - /// attributes are considered better. + /// attributes are considred better. Attribute, /// Sorted by the similarity of the matched words with the query words. Exactness, @@ -41,46 +43,29 @@ impl Criterion { impl FromStr for Criterion { type Err = Error; - fn from_str(text: &str) -> Result { - match text { + fn from_str(txt: &str) -> Result { + match txt { "words" => Ok(Criterion::Words), "typo" => Ok(Criterion::Typo), - "sort" => Ok(Criterion::Sort), "proximity" => Ok(Criterion::Proximity), "attribute" => Ok(Criterion::Attribute), "exactness" => Ok(Criterion::Exactness), - text => match AscDesc::from_str(text) { - Ok(AscDesc::Asc(field)) => Ok(Criterion::Asc(field)), - Ok(AscDesc::Desc(field)) => Ok(Criterion::Desc(field)), - Err(error) => Err(error.into()), - }, - } - } -} - -#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] -pub enum AscDesc { - Asc(String), - Desc(String), -} - -impl AscDesc { - pub fn field(&self) -> &str { - match self { - AscDesc::Asc(field) => field, - AscDesc::Desc(field) => field, - } - } -} - -impl FromStr for AscDesc { - type Err = UserError; - - fn from_str(text: &str) -> Result { - match text.rsplit_once(':') { - Some((field_name, "asc")) => Ok(AscDesc::Asc(field_name.to_string())), - Some((field_name, "desc")) => Ok(AscDesc::Desc(field_name.to_string())), - _ => Err(UserError::InvalidCriterionName { name: text.to_string() }), + text => { + let caps = ASC_DESC_REGEX + .captures(text) + .ok_or_else(|| UserError::InvalidCriterionName { name: text.to_string() })?; + let order = caps.get(1).unwrap().as_str(); + let field_name = caps.get(2).unwrap().as_str(); + match order { + "asc" => Ok(Criterion::Asc(field_name.to_string())), + "desc" => Ok(Criterion::Desc(field_name.to_string())), + text => { + return Err( + UserError::InvalidCriterionName { name: text.to_string() }.into() + ) + } + } + } } } } @@ -89,7 +74,6 @@ pub fn default_criteria() -> Vec { vec![ Criterion::Words, Criterion::Typo, - Criterion::Sort, Criterion::Proximity, Criterion::Attribute, Criterion::Exactness, @@ -103,12 +87,11 @@ impl fmt::Display for Criterion { match self { Words => f.write_str("words"), Typo => f.write_str("typo"), - Sort => f.write_str("sort"), Proximity => f.write_str("proximity"), Attribute => f.write_str("attribute"), Exactness => f.write_str("exactness"), - Asc(attr) => write!(f, "{}:asc", attr), - Desc(attr) => write!(f, "{}:desc", attr), + Asc(attr) => write!(f, "asc({})", attr), + Desc(attr) => write!(f, "desc({})", attr), } } } diff --git a/milli/src/error.rs b/milli/src/error.rs index 9bda74631..713935869 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -58,7 +58,6 @@ pub enum UserError { InvalidFacetsDistribution { invalid_facets_name: HashSet }, InvalidFilter(pest::error::Error), InvalidFilterAttribute(pest::error::Error), - InvalidSortableAttribute { field: String, valid_fields: HashSet }, InvalidStoreFile, MaxDatabaseSizeReached, MissingDocumentId { document: Object }, @@ -227,15 +226,6 @@ only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and undersco ) } Self::InvalidFilterAttribute(error) => error.fmt(f), - Self::InvalidSortableAttribute { field, valid_fields } => { - let valid_names = - valid_fields.iter().map(AsRef::as_ref).collect::>().join(", "); - write!( - f, - "Attribute {} is not sortable, available sortable attributes are: {}", - field, valid_names - ) - } Self::MissingDocumentId { document } => { let json = serde_json::to_string(document).unwrap(); write!(f, "document doesn't have an identifier {}", json) diff --git a/milli/src/index.rs b/milli/src/index.rs index e2ab51a1c..120bcbadf 100644 --- a/milli/src/index.rs +++ b/milli/src/index.rs @@ -28,7 +28,6 @@ pub mod main_key { pub const DISTINCT_FIELD_KEY: &str = "distinct-field-key"; pub const DOCUMENTS_IDS_KEY: &str = "documents-ids"; pub const FILTERABLE_FIELDS_KEY: &str = "filterable-fields"; - pub const SORTABLE_FIELDS_KEY: &str = "sortable-fields"; pub const FIELD_DISTRIBUTION_KEY: &str = "fields-distribution"; pub const FIELDS_IDS_MAP_KEY: &str = "fields-ids-map"; pub const HARD_EXTERNAL_DOCUMENTS_IDS_KEY: &str = "hard-external-documents-ids"; @@ -447,45 +446,13 @@ impl Index { Ok(fields_ids) } - /* sortable fields */ - - /// Writes the sortable fields names in the database. - pub(crate) fn put_sortable_fields( - &self, - wtxn: &mut RwTxn, - fields: &HashSet, - ) -> heed::Result<()> { - self.main.put::<_, Str, SerdeJson<_>>(wtxn, main_key::SORTABLE_FIELDS_KEY, fields) - } - - /// Deletes the sortable fields ids in the database. - pub(crate) fn delete_sortable_fields(&self, wtxn: &mut RwTxn) -> heed::Result { - self.main.delete::<_, Str>(wtxn, main_key::SORTABLE_FIELDS_KEY) - } - - /// Returns the sortable fields names. - pub fn sortable_fields(&self, rtxn: &RoTxn) -> heed::Result> { - Ok(self - .main - .get::<_, Str, SerdeJson<_>>(rtxn, main_key::SORTABLE_FIELDS_KEY)? - .unwrap_or_default()) - } - - /// Identical to `sortable_fields`, but returns ids instead. - pub fn sortable_fields_ids(&self, rtxn: &RoTxn) -> Result> { - let fields = self.sortable_fields(rtxn)?; - let fields_ids_map = self.fields_ids_map(rtxn)?; - Ok(fields.into_iter().filter_map(|name| fields_ids_map.id(&name)).collect()) - } - /* faceted documents ids */ /// Returns the faceted fields names. /// - /// Faceted fields are the union of all the filterable, sortable, distinct, and Asc/Desc fields. + /// Faceted fields are the union of all the filterable, distinct, and Asc/Desc fields. pub fn faceted_fields(&self, rtxn: &RoTxn) -> Result> { let filterable_fields = self.filterable_fields(rtxn)?; - let sortable_fields = self.sortable_fields(rtxn)?; let distinct_field = self.distinct_field(rtxn)?; let asc_desc_fields = self.criteria(rtxn)?.into_iter().filter_map(|criterion| match criterion { @@ -494,7 +461,6 @@ impl Index { }); let mut faceted_fields = filterable_fields; - faceted_fields.extend(sortable_fields); faceted_fields.extend(asc_desc_fields); if let Some(field) = distinct_field { faceted_fields.insert(field.to_owned()); diff --git a/milli/src/lib.rs b/milli/src/lib.rs index 2b0bd2ed4..f3bababf6 100644 --- a/milli/src/lib.rs +++ b/milli/src/lib.rs @@ -22,7 +22,7 @@ use std::result::Result as StdResult; use fxhash::{FxHasher32, FxHasher64}; use serde_json::{Map, Value}; -pub use self::criterion::{default_criteria, AscDesc, Criterion}; +pub use self::criterion::{default_criteria, Criterion}; pub use self::error::{ Error, FieldIdMapMissingEntry, InternalError, SerializationError, UserError, }; diff --git a/milli/src/search/criteria/asc_desc.rs b/milli/src/search/criteria/asc_desc.rs index 6d50c1bb5..4a664d042 100644 --- a/milli/src/search/criteria/asc_desc.rs +++ b/milli/src/search/criteria/asc_desc.rs @@ -7,7 +7,7 @@ use roaring::RoaringBitmap; use super::{Criterion, CriterionParameters, CriterionResult}; use crate::search::criteria::{resolve_query_tree, CriteriaBuilder}; -use crate::search::facet::{FacetNumberIter, FacetStringIter}; +use crate::search::facet::FacetNumberIter; use crate::search::query_tree::Operation; use crate::{FieldId, Index, Result}; @@ -20,7 +20,7 @@ pub struct AscDesc<'t> { rtxn: &'t heed::RoTxn<'t>, field_name: String, field_id: Option, - is_ascending: bool, + ascending: bool, query_tree: Option, candidates: Box> + 't>, allowed_candidates: RoaringBitmap, @@ -53,16 +53,12 @@ impl<'t> AscDesc<'t> { rtxn: &'t heed::RoTxn, parent: Box, field_name: String, - is_ascending: bool, + ascending: bool, ) -> Result { let fields_ids_map = index.fields_ids_map(rtxn)?; let field_id = fields_ids_map.id(&field_name); let faceted_candidates = match field_id { - Some(field_id) => { - let number_faceted = index.number_faceted_documents_ids(rtxn, field_id)?; - let string_faceted = index.string_faceted_documents_ids(rtxn, field_id)?; - number_faceted | string_faceted - } + Some(field_id) => index.number_faceted_documents_ids(rtxn, field_id)?, None => RoaringBitmap::default(), }; @@ -71,7 +67,7 @@ impl<'t> AscDesc<'t> { rtxn, field_name, field_id, - is_ascending, + ascending, query_tree: None, candidates: Box::new(std::iter::empty()), allowed_candidates: RoaringBitmap::new(), @@ -91,7 +87,7 @@ impl<'t> Criterion for AscDesc<'t> { loop { debug!( "Facet {}({}) iteration", - if self.is_ascending { "Asc" } else { "Desc" }, + if self.ascending { "Asc" } else { "Desc" }, self.field_name ); @@ -140,7 +136,7 @@ impl<'t> Criterion for AscDesc<'t> { self.index, self.rtxn, field_id, - self.is_ascending, + self.ascending, candidates & &self.faceted_candidates, )?, None => Box::new(std::iter::empty()), @@ -171,49 +167,31 @@ fn facet_ordered<'t>( index: &'t Index, rtxn: &'t heed::RoTxn, field_id: FieldId, - is_ascending: bool, + ascending: bool, candidates: RoaringBitmap, ) -> Result> + 't>> { if candidates.len() <= CANDIDATES_THRESHOLD { - let number_iter = iterative_facet_number_ordered_iter( - index, - rtxn, - field_id, - is_ascending, - candidates.clone(), - )?; - let string_iter = - iterative_facet_string_ordered_iter(index, rtxn, field_id, is_ascending, candidates)?; - Ok(Box::new(number_iter.chain(string_iter).map(Ok)) as Box>) + let iter = iterative_facet_ordered_iter(index, rtxn, field_id, ascending, candidates)?; + Ok(Box::new(iter.map(Ok)) as Box>) } else { - let facet_number_fn = if is_ascending { + let facet_fn = if ascending { FacetNumberIter::new_reducing } else { FacetNumberIter::new_reverse_reducing }; - let number_iter = facet_number_fn(rtxn, index, field_id, candidates.clone())? - .map(|res| res.map(|(_, docids)| docids)); - - let facet_string_fn = if is_ascending { - FacetStringIter::new_reducing - } else { - FacetStringIter::new_reverse_reducing - }; - let string_iter = facet_string_fn(rtxn, index, field_id, candidates)? - .map(|res| res.map(|(_, _, docids)| docids)); - - Ok(Box::new(number_iter.chain(string_iter))) + let iter = facet_fn(rtxn, index, field_id, candidates)?; + Ok(Box::new(iter.map(|res| res.map(|(_, docids)| docids)))) } } -/// Fetch the whole list of candidates facet number values one by one and order them by it. +/// Fetch the whole list of candidates facet values one by one and order them by it. /// /// This function is fast when the amount of candidates to rank is small. -fn iterative_facet_number_ordered_iter<'t>( +fn iterative_facet_ordered_iter<'t>( index: &'t Index, rtxn: &'t heed::RoTxn, field_id: FieldId, - is_ascending: bool, + ascending: bool, candidates: RoaringBitmap, ) -> Result + 't> { let mut docids_values = Vec::with_capacity(candidates.len() as usize); @@ -221,14 +199,14 @@ fn iterative_facet_number_ordered_iter<'t>( let left = (field_id, docid, f64::MIN); let right = (field_id, docid, f64::MAX); let mut iter = index.field_id_docid_facet_f64s.range(rtxn, &(left..=right))?; - let entry = if is_ascending { iter.next() } else { iter.last() }; + let entry = if ascending { iter.next() } else { iter.last() }; if let Some(((_, _, value), ())) = entry.transpose()? { docids_values.push((docid, OrderedFloat(value))); } } docids_values.sort_unstable_by_key(|(_, v)| *v); let iter = docids_values.into_iter(); - let iter = if is_ascending { + let iter = if ascending { Box::new(iter) as Box> } else { Box::new(iter.rev()) @@ -238,49 +216,7 @@ fn iterative_facet_number_ordered_iter<'t>( // required to collect the result into an owned collection (a Vec). // https://github.com/rust-itertools/itertools/issues/499 let vec: Vec<_> = iter - .group_by(|(_, v)| *v) - .into_iter() - .map(|(_, ids)| ids.map(|(id, _)| id).collect()) - .collect(); - - Ok(vec.into_iter()) -} - -/// Fetch the whole list of candidates facet string values one by one and order them by it. -/// -/// This function is fast when the amount of candidates to rank is small. -fn iterative_facet_string_ordered_iter<'t>( - index: &'t Index, - rtxn: &'t heed::RoTxn, - field_id: FieldId, - is_ascending: bool, - candidates: RoaringBitmap, -) -> Result + 't> { - let mut docids_values = Vec::with_capacity(candidates.len() as usize); - for docid in candidates.iter() { - let left = (field_id, docid, ""); - let right = (field_id, docid.saturating_add(1), ""); - // FIXME Doing this means that it will never be possible to retrieve - // the document with id 2^32, not sure this is a real problem. - let mut iter = index.field_id_docid_facet_strings.range(rtxn, &(left..right))?; - let entry = if is_ascending { iter.next() } else { iter.last() }; - if let Some(((_, _, value), _)) = entry.transpose()? { - docids_values.push((docid, value)); - } - } - docids_values.sort_unstable_by_key(|(_, v)| *v); - let iter = docids_values.into_iter(); - let iter = if is_ascending { - Box::new(iter) as Box> - } else { - Box::new(iter.rev()) - }; - - // The itertools GroupBy iterator doesn't provide an owned version, we are therefore - // required to collect the result into an owned collection (a Vec). - // https://github.com/rust-itertools/itertools/issues/499 - let vec: Vec<_> = iter - .group_by(|(_, v)| *v) + .group_by(|(_, v)| v.clone()) .into_iter() .map(|(_, ids)| ids.map(|(id, _)| id).collect()) .collect(); diff --git a/milli/src/search/criteria/mod.rs b/milli/src/search/criteria/mod.rs index 61b0fe049..2ba3b388f 100644 --- a/milli/src/search/criteria/mod.rs +++ b/milli/src/search/criteria/mod.rs @@ -12,7 +12,6 @@ use self::r#final::Final; use self::typo::Typo; use self::words::Words; use super::query_tree::{Operation, PrimitiveQueryPart, Query, QueryKind}; -use crate::criterion::AscDesc as AscDescName; use crate::search::{word_derivations, WordDerivationsCache}; use crate::{DocumentId, FieldId, Index, Result, TreeLevel}; @@ -274,7 +273,6 @@ impl<'t> CriteriaBuilder<'t> { query_tree: Option, primitive_query: Option>, filtered_candidates: Option, - sort_criteria: Option>, ) -> Result> { use crate::criterion::Criterion as Name; @@ -284,30 +282,8 @@ impl<'t> CriteriaBuilder<'t> { Box::new(Initial::new(query_tree, filtered_candidates)) as Box; for name in self.index.criteria(&self.rtxn)? { criterion = match name { - Name::Words => Box::new(Words::new(self, criterion)), Name::Typo => Box::new(Typo::new(self, criterion)), - Name::Sort => match sort_criteria { - Some(ref sort_criteria) => { - for asc_desc in sort_criteria { - criterion = match asc_desc { - AscDescName::Asc(field) => Box::new(AscDesc::asc( - &self.index, - &self.rtxn, - criterion, - field.to_string(), - )?), - AscDescName::Desc(field) => Box::new(AscDesc::desc( - &self.index, - &self.rtxn, - criterion, - field.to_string(), - )?), - }; - } - criterion - } - None => criterion, - }, + Name::Words => Box::new(Words::new(self, criterion)), Name::Proximity => Box::new(Proximity::new(self, criterion)), Name::Attribute => Box::new(Attribute::new(self, criterion)), Name::Exactness => Box::new(Exactness::new(self, criterion, &primitive_query)?), diff --git a/milli/src/search/facet/facet_string.rs b/milli/src/search/facet/facet_string.rs index 927602c98..ed5322607 100644 --- a/milli/src/search/facet/facet_string.rs +++ b/milli/src/search/facet/facet_string.rs @@ -131,7 +131,7 @@ use std::ops::Bound::{Excluded, Included, Unbounded}; use either::{Either, Left, Right}; use heed::types::{ByteSlice, DecodeIgnore}; -use heed::{Database, LazyDecode, RoRange, RoRevRange}; +use heed::{Database, LazyDecode, RoRange}; use roaring::RoaringBitmap; use crate::heed_codec::facet::{ @@ -206,65 +206,6 @@ impl<'t> Iterator for FacetStringGroupRange<'t> { } } -pub struct FacetStringGroupRevRange<'t> { - iter: RoRevRange< - 't, - FacetLevelValueU32Codec, - LazyDecode>, - >, - end: Bound, -} - -impl<'t> FacetStringGroupRevRange<'t> { - pub fn new( - rtxn: &'t heed::RoTxn, - db: Database, - field_id: FieldId, - level: NonZeroU8, - left: Bound, - right: Bound, - ) -> heed::Result> { - let db = db.remap_types::< - FacetLevelValueU32Codec, - FacetStringZeroBoundsValueCodec, - >(); - let left_bound = match left { - Included(left) => Included((field_id, level, left, u32::MIN)), - Excluded(left) => Excluded((field_id, level, left, u32::MIN)), - Unbounded => Included((field_id, level, u32::MIN, u32::MIN)), - }; - let right_bound = Included((field_id, level, u32::MAX, u32::MAX)); - let iter = db.lazily_decode_data().rev_range(rtxn, &(left_bound, right_bound))?; - Ok(FacetStringGroupRevRange { iter, end: right }) - } -} - -impl<'t> Iterator for FacetStringGroupRevRange<'t> { - type Item = heed::Result<((NonZeroU8, u32, u32), (Option<(&'t str, &'t str)>, RoaringBitmap))>; - - fn next(&mut self) -> Option { - match self.iter.next() { - Some(Ok(((_fid, level, left, right), docids))) => { - let must_be_returned = match self.end { - Included(end) => right <= end, - Excluded(end) => right < end, - Unbounded => true, - }; - if must_be_returned { - match docids.decode() { - Ok((bounds, docids)) => Some(Ok(((level, left, right), (bounds, docids)))), - Err(e) => Some(Err(e)), - } - } else { - None - } - } - Some(Err(e)) => Some(Err(e)), - None => None, - } - } -} - /// An iterator that is used to explore the level 0 of the facets string database. /// /// It yields the facet string and the roaring bitmap associated with it. @@ -339,81 +280,6 @@ impl<'t> Iterator for FacetStringLevelZeroRange<'t> { } } -pub struct FacetStringLevelZeroRevRange<'t> { - iter: RoRevRange< - 't, - FacetStringLevelZeroCodec, - FacetStringLevelZeroValueCodec, - >, -} - -impl<'t> FacetStringLevelZeroRevRange<'t> { - pub fn new( - rtxn: &'t heed::RoTxn, - db: Database, - field_id: FieldId, - left: Bound<&str>, - right: Bound<&str>, - ) -> heed::Result> { - fn encode_value<'a>(buffer: &'a mut Vec, field_id: FieldId, value: &str) -> &'a [u8] { - buffer.extend_from_slice(&field_id.to_be_bytes()); - buffer.push(0); - buffer.extend_from_slice(value.as_bytes()); - &buffer[..] - } - - let mut left_buffer = Vec::new(); - let left_bound = match left { - Included(value) => Included(encode_value(&mut left_buffer, field_id, value)), - Excluded(value) => Excluded(encode_value(&mut left_buffer, field_id, value)), - Unbounded => { - left_buffer.extend_from_slice(&field_id.to_be_bytes()); - left_buffer.push(0); - Included(&left_buffer[..]) - } - }; - - let mut right_buffer = Vec::new(); - let right_bound = match right { - Included(value) => Included(encode_value(&mut right_buffer, field_id, value)), - Excluded(value) => Excluded(encode_value(&mut right_buffer, field_id, value)), - Unbounded => { - right_buffer.extend_from_slice(&field_id.to_be_bytes()); - right_buffer.push(1); // we must only get the level 0 - Excluded(&right_buffer[..]) - } - }; - - let iter = db - .remap_key_type::() - .rev_range(rtxn, &(left_bound, right_bound))? - .remap_types::< - FacetStringLevelZeroCodec, - FacetStringLevelZeroValueCodec - >(); - - Ok(FacetStringLevelZeroRevRange { iter }) - } -} - -impl<'t> Iterator for FacetStringLevelZeroRevRange<'t> { - type Item = heed::Result<(&'t str, &'t str, RoaringBitmap)>; - - fn next(&mut self) -> Option { - match self.iter.next() { - Some(Ok(((_fid, normalized), (original, docids)))) => { - Some(Ok((normalized, original, docids))) - } - Some(Err(e)) => Some(Err(e)), - None => None, - } - } -} - -type EitherStringRange<'t> = Either, FacetStringLevelZeroRange<'t>>; -type EitherStringRevRange<'t> = - Either, FacetStringLevelZeroRevRange<'t>>; - /// An iterator that is used to explore the facet strings level by level, /// it will only return facets strings that are associated with the /// candidates documents ids given. @@ -421,45 +287,12 @@ pub struct FacetStringIter<'t> { rtxn: &'t heed::RoTxn<'t>, db: Database, field_id: FieldId, - level_iters: Vec<(RoaringBitmap, Either, EitherStringRevRange<'t>>)>, + level_iters: + Vec<(RoaringBitmap, Either, FacetStringLevelZeroRange<'t>>)>, must_reduce: bool, } impl<'t> FacetStringIter<'t> { - pub fn new_reducing( - rtxn: &'t heed::RoTxn, - index: &'t Index, - field_id: FieldId, - documents_ids: RoaringBitmap, - ) -> heed::Result> { - let db = index.facet_id_string_docids.remap_types::(); - let highest_iter = Self::highest_iter(rtxn, index, db, field_id)?; - Ok(FacetStringIter { - rtxn, - db, - field_id, - level_iters: vec![(documents_ids, Left(highest_iter))], - must_reduce: true, - }) - } - - pub fn new_reverse_reducing( - rtxn: &'t heed::RoTxn, - index: &'t Index, - field_id: FieldId, - documents_ids: RoaringBitmap, - ) -> heed::Result> { - let db = index.facet_id_string_docids.remap_types::(); - let highest_reverse_iter = Self::highest_reverse_iter(rtxn, index, db, field_id)?; - Ok(FacetStringIter { - rtxn, - db, - field_id, - level_iters: vec![(documents_ids, Right(highest_reverse_iter))], - must_reduce: true, - }) - } - pub fn new_non_reducing( rtxn: &'t heed::RoTxn, index: &'t Index, @@ -467,12 +300,30 @@ impl<'t> FacetStringIter<'t> { documents_ids: RoaringBitmap, ) -> heed::Result> { let db = index.facet_id_string_docids.remap_types::(); - let highest_iter = Self::highest_iter(rtxn, index, db, field_id)?; + let highest_level = Self::highest_level(rtxn, db, field_id)?.unwrap_or(0); + let highest_iter = match NonZeroU8::new(highest_level) { + Some(highest_level) => Left(FacetStringGroupRange::new( + rtxn, + index.facet_id_string_docids, + field_id, + highest_level, + Unbounded, + Unbounded, + )?), + None => Right(FacetStringLevelZeroRange::new( + rtxn, + index.facet_id_string_docids, + field_id, + Unbounded, + Unbounded, + )?), + }; + Ok(FacetStringIter { rtxn, db, field_id, - level_iters: vec![(documents_ids, Left(highest_iter))], + level_iters: vec![(documents_ids, highest_iter)], must_reduce: false, }) } @@ -489,62 +340,6 @@ impl<'t> FacetStringIter<'t> { .transpose()? .map(|(key_bytes, _)| key_bytes[2])) // the level is the third bit } - - fn highest_iter( - rtxn: &'t heed::RoTxn, - index: &'t Index, - db: Database, - field_id: FieldId, - ) -> heed::Result, FacetStringLevelZeroRange<'t>>> { - let highest_level = Self::highest_level(rtxn, db, field_id)?.unwrap_or(0); - match NonZeroU8::new(highest_level) { - Some(highest_level) => FacetStringGroupRange::new( - rtxn, - index.facet_id_string_docids, - field_id, - highest_level, - Unbounded, - Unbounded, - ) - .map(Left), - None => FacetStringLevelZeroRange::new( - rtxn, - index.facet_id_string_docids, - field_id, - Unbounded, - Unbounded, - ) - .map(Right), - } - } - - fn highest_reverse_iter( - rtxn: &'t heed::RoTxn, - index: &'t Index, - db: Database, - field_id: FieldId, - ) -> heed::Result, FacetStringLevelZeroRevRange<'t>>> { - let highest_level = Self::highest_level(rtxn, db, field_id)?.unwrap_or(0); - match NonZeroU8::new(highest_level) { - Some(highest_level) => FacetStringGroupRevRange::new( - rtxn, - index.facet_id_string_docids, - field_id, - highest_level, - Unbounded, - Unbounded, - ) - .map(Left), - None => FacetStringLevelZeroRevRange::new( - rtxn, - index.facet_id_string_docids, - field_id, - Unbounded, - Unbounded, - ) - .map(Right), - } - } } impl<'t> Iterator for FacetStringIter<'t> { @@ -553,21 +348,6 @@ impl<'t> Iterator for FacetStringIter<'t> { fn next(&mut self) -> Option { 'outer: loop { let (documents_ids, last) = self.level_iters.last_mut()?; - let is_ascending = last.is_left(); - - // We remap the different iterator types to make - // the algorithm less complex to understand. - let last = match last { - Left(ascending) => match ascending { - Left(last) => Left(Left(last)), - Right(last) => Right(Left(last)), - }, - Right(descending) => match descending { - Left(last) => Left(Right(last)), - Right(last) => Right(Right(last)), - }, - }; - match last { Left(last) => { for result in last { @@ -579,50 +359,24 @@ impl<'t> Iterator for FacetStringIter<'t> { *documents_ids -= &docids; } - let result = if is_ascending { - match string_bounds { - Some((left, right)) => { - FacetStringLevelZeroRevRange::new( - self.rtxn, - self.db, - self.field_id, - Included(left), - Included(right), - ) - .map(Right) - } - None => FacetStringGroupRevRange::new( - self.rtxn, - self.db, - self.field_id, - NonZeroU8::new(level.get() - 1).unwrap(), - Included(left), - Included(right), - ) - .map(Left), - } - .map(Right) - } else { - match string_bounds { - Some((left, right)) => FacetStringLevelZeroRange::new( - self.rtxn, - self.db, - self.field_id, - Included(left), - Included(right), - ) - .map(Right), - None => FacetStringGroupRange::new( - self.rtxn, - self.db, - self.field_id, - NonZeroU8::new(level.get() - 1).unwrap(), - Included(left), - Included(right), - ) - .map(Left), - } - .map(Left) + let result = match string_bounds { + Some((left, right)) => FacetStringLevelZeroRange::new( + self.rtxn, + self.db, + self.field_id, + Included(left), + Included(right), + ) + .map(Right), + None => FacetStringGroupRange::new( + self.rtxn, + self.db, + self.field_id, + NonZeroU8::new(level.get() - 1).unwrap(), + Included(left), + Included(right), + ) + .map(Left), }; match result { diff --git a/milli/src/search/mod.rs b/milli/src/search/mod.rs index 23e5c1834..871f464ef 100644 --- a/milli/src/search/mod.rs +++ b/milli/src/search/mod.rs @@ -18,8 +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::criterion::AscDesc; -use crate::error::UserError; use crate::search::criteria::r#final::{Final, FinalResult}; use crate::{DocumentId, Index, Result}; @@ -39,7 +37,6 @@ pub struct Search<'a> { filter: Option, offset: usize, limit: usize, - sort_criteria: Option>, optional_words: bool, authorize_typos: bool, words_limit: usize, @@ -54,7 +51,6 @@ impl<'a> Search<'a> { filter: None, offset: 0, limit: 20, - sort_criteria: None, optional_words: true, authorize_typos: true, words_limit: 10, @@ -78,11 +74,6 @@ impl<'a> Search<'a> { self } - pub fn sort_criteria(&mut self, criteria: Vec) -> &mut Search<'a> { - self.sort_criteria = Some(criteria); - self - } - pub fn optional_words(&mut self, value: bool) -> &mut Search<'a> { self.optional_words = value; self @@ -143,29 +134,8 @@ impl<'a> Search<'a> { None => MatchingWords::default(), }; - // We check that we are allowed to use the sort criteria, we check - // that they are declared in the sortable fields. - let sortable_fields = self.index.sortable_fields(self.rtxn)?; - if let Some(sort_criteria) = &self.sort_criteria { - for asc_desc in sort_criteria { - let field = asc_desc.field(); - if !sortable_fields.contains(field) { - return Err(UserError::InvalidSortableAttribute { - field: field.to_string(), - valid_fields: sortable_fields, - } - .into()); - } - } - } - let criteria_builder = criteria::CriteriaBuilder::new(self.rtxn, self.index)?; - let criteria = criteria_builder.build( - query_tree, - primitive_query, - filtered_candidates, - self.sort_criteria.clone(), - )?; + let criteria = criteria_builder.build(query_tree, primitive_query, filtered_candidates)?; match self.index.distinct_field(self.rtxn)? { None => self.perform_sort(NoopDistinct, matching_words, criteria), @@ -229,7 +199,6 @@ impl fmt::Debug for Search<'_> { filter, offset, limit, - sort_criteria, optional_words, authorize_typos, words_limit, @@ -241,7 +210,6 @@ impl fmt::Debug for Search<'_> { .field("filter", filter) .field("offset", offset) .field("limit", limit) - .field("sort_criteria", sort_criteria) .field("optional_words", optional_words) .field("authorize_typos", authorize_typos) .field("words_limit", words_limit) diff --git a/milli/src/update/settings.rs b/milli/src/update/settings.rs index c0b5e4549..07bdfd6fa 100644 --- a/milli/src/update/settings.rs +++ b/milli/src/update/settings.rs @@ -75,7 +75,6 @@ pub struct Settings<'a, 't, 'u, 'i> { searchable_fields: Setting>, displayed_fields: Setting>, filterable_fields: Setting>, - sortable_fields: Setting>, criteria: Setting>, stop_words: Setting>, distinct_field: Setting, @@ -103,7 +102,6 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { searchable_fields: Setting::NotSet, displayed_fields: Setting::NotSet, filterable_fields: Setting::NotSet, - sortable_fields: Setting::NotSet, criteria: Setting::NotSet, stop_words: Setting::NotSet, distinct_field: Setting::NotSet, @@ -137,10 +135,6 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { self.filterable_fields = Setting::Set(names); } - pub fn set_sortable_fields(&mut self, names: HashSet) { - self.sortable_fields = Setting::Set(names); - } - pub fn reset_criteria(&mut self) { self.criteria = Setting::Reset; } @@ -398,23 +392,6 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { Ok(()) } - fn update_sortable(&mut self) -> Result<()> { - match self.sortable_fields { - Setting::Set(ref fields) => { - let mut new_fields = HashSet::new(); - for name in fields { - new_fields.insert(name.clone()); - } - self.index.put_sortable_fields(self.wtxn, &new_fields)?; - } - Setting::Reset => { - self.index.delete_sortable_fields(self.wtxn)?; - } - Setting::NotSet => (), - } - Ok(()) - } - fn update_criteria(&mut self) -> Result<()> { match self.criteria { Setting::Set(ref fields) => { @@ -469,7 +446,6 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { self.update_displayed()?; self.update_filterable()?; - self.update_sortable()?; self.update_distinct_field()?; self.update_criteria()?; self.update_primary_key()?; @@ -743,7 +719,7 @@ mod tests { let mut builder = Settings::new(&mut wtxn, &index, 0); // Don't display the generated `id` field. builder.set_displayed_fields(vec![S("name")]); - builder.set_criteria(vec![S("age:asc")]); + builder.set_criteria(vec![S("asc(age)")]); builder.execute(|_, _| ()).unwrap(); // Then index some documents. @@ -977,7 +953,7 @@ mod tests { let mut builder = Settings::new(&mut wtxn, &index, 0); builder.set_displayed_fields(vec!["hello".to_string()]); builder.set_filterable_fields(hashset! { S("age"), S("toto") }); - builder.set_criteria(vec!["toto:asc".to_string()]); + builder.set_criteria(vec!["asc(toto)".to_string()]); builder.execute(|_, _| ()).unwrap(); wtxn.commit().unwrap(); @@ -1014,7 +990,7 @@ mod tests { let mut builder = Settings::new(&mut wtxn, &index, 0); builder.set_displayed_fields(vec!["hello".to_string()]); // It is only Asc(toto), there is a facet database but it is denied to filter with toto. - builder.set_criteria(vec!["toto:asc".to_string()]); + builder.set_criteria(vec!["asc(toto)".to_string()]); builder.execute(|_, _| ()).unwrap(); wtxn.commit().unwrap(); diff --git a/milli/tests/assets/test_set.ndjson b/milli/tests/assets/test_set.ndjson index 89d9f1109..599d479ed 100644 --- a/milli/tests/assets/test_set.ndjson +++ b/milli/tests/assets/test_set.ndjson @@ -1,17 +1,17 @@ -{"id":"A","word_rank":0,"typo_rank":1,"proximity_rank":15,"attribute_rank":505,"exact_rank":5,"asc_desc_rank":0,"sort_by_rank":0,"title":"hell o","description":"hell o is the fourteenth episode of the american television series glee performing songs with this word","tag":"blue","":""} -{"id":"B","word_rank":2,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":4,"asc_desc_rank":1,"sort_by_rank":2,"title":"hello","description":"hello is a song recorded by english singer songwriter adele","tag":"red","":""} -{"id":"C","word_rank":0,"typo_rank":1,"proximity_rank":8,"attribute_rank":336,"exact_rank":4,"asc_desc_rank":2,"sort_by_rank":0,"title":"hell on earth","description":"hell on earth is the third studio album by american hip hop duo mobb deep","tag":"blue","":""} -{"id":"D","word_rank":0,"typo_rank":1,"proximity_rank":10,"attribute_rank":757,"exact_rank":4,"asc_desc_rank":3,"sort_by_rank":2,"title":"hell on wheels tv series","description":"the construction of the first transcontinental railroad across the united states in the world","tag":"red","":""} -{"id":"E","word_rank":2,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":4,"asc_desc_rank":4,"sort_by_rank":1,"title":"hello kitty","description":"also known by her full name kitty white is a fictional character produced by the japanese company sanrio","tag":"green","":""} -{"id":"F","word_rank":2,"typo_rank":1,"proximity_rank":0,"attribute_rank":1017,"exact_rank":5,"asc_desc_rank":5,"sort_by_rank":0,"title":"laptop orchestra","description":"a laptop orchestra lork or lo is a chamber music ensemble consisting primarily of laptops like helo huddersfield experimental laptop orchestra","tag":"blue","":""} -{"id":"G","word_rank":1,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":3,"asc_desc_rank":5,"sort_by_rank":2,"title":"hello world film","description":"hello world is a 2019 japanese animated sci fi romantic drama film directed by tomohiko ito and produced by graphinica","tag":"red","":""} -{"id":"H","word_rank":1,"typo_rank":0,"proximity_rank":1,"attribute_rank":0,"exact_rank":3,"asc_desc_rank":4,"sort_by_rank":1,"title":"world hello day","description":"holiday observed on november 21 to express that conflicts should be resolved through communication rather than the use of force","tag":"green","":""} -{"id":"I","word_rank":0,"typo_rank":0,"proximity_rank":8,"attribute_rank":338,"exact_rank":3,"asc_desc_rank":3,"sort_by_rank":0,"title":"hello world song","description":"hello world is a song written by tom douglas tony lane and david lee and recorded by american country music group lady antebellum","tag":"blue","":""} -{"id":"J","word_rank":1,"typo_rank":0,"proximity_rank":1,"attribute_rank":1,"exact_rank":3,"asc_desc_rank":2,"sort_by_rank":1,"title":"hello cruel world","description":"hello cruel world is an album by new zealand band tall dwarfs","tag":"green","":""} -{"id":"K","word_rank":0,"typo_rank":2,"proximity_rank":9,"attribute_rank":670,"exact_rank":5,"asc_desc_rank":1,"sort_by_rank":2,"title":"ello creation system","description":"in few word ello was a construction toy created by the american company mattel to engage girls in construction play","tag":"red","":""} -{"id":"L","word_rank":0,"typo_rank":0,"proximity_rank":2,"attribute_rank":250,"exact_rank":4,"asc_desc_rank":0,"sort_by_rank":0,"title":"good morning world","description":"good morning world is an american sitcom broadcast on cbs tv during the 1967 1968 season","tag":"blue","":""} -{"id":"M","word_rank":0,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":0,"asc_desc_rank":0,"sort_by_rank":2,"title":"hello world america","description":"a perfect match for a perfect engine using the query hello world america","tag":"red","":""} -{"id":"N","word_rank":0,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":1,"asc_desc_rank":4,"sort_by_rank":1,"title":"hello world america unleashed","description":"a very good match for a very good engine using the query hello world america","tag":"green","":""} -{"id":"O","word_rank":0,"typo_rank":0,"proximity_rank":0,"attribute_rank":10,"exact_rank":0,"asc_desc_rank":6,"sort_by_rank":0,"title":"a perfect match for a perfect engine using the query hello world america","description":"hello world america","tag":"blue","":""} -{"id":"P","word_rank":0,"typo_rank":0,"proximity_rank":0,"attribute_rank":12,"exact_rank":1,"asc_desc_rank":3,"sort_by_rank":2,"title":"a very good match for a very good engine using the query hello world america","description":"hello world america unleashed","tag":"red","":""} -{"id":"Q","word_rank":1,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":3,"asc_desc_rank":2,"sort_by_rank":1,"title":"hello world","description":"a hello world program generally is a computer program that outputs or displays the message hello world","tag":"green","":""} +{"id":"A","word_rank":0,"typo_rank":1,"proximity_rank":15,"attribute_rank":505,"exact_rank":5,"asc_desc_rank":0,"title":"hell o","description":"hell o is the fourteenth episode of the american television series glee performing songs with this word","tag":"blue","":""} +{"id":"B","word_rank":2,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":4,"asc_desc_rank":1,"title":"hello","description":"hello is a song recorded by english singer songwriter adele","tag":"red","":""} +{"id":"C","word_rank":0,"typo_rank":1,"proximity_rank":8,"attribute_rank":336,"exact_rank":4,"asc_desc_rank":2,"title":"hell on earth","description":"hell on earth is the third studio album by american hip hop duo mobb deep","tag":"blue","":""} +{"id":"D","word_rank":0,"typo_rank":1,"proximity_rank":10,"attribute_rank":757,"exact_rank":4,"asc_desc_rank":3,"title":"hell on wheels tv series","description":"the construction of the first transcontinental railroad across the united states in the world","tag":"red","":""} +{"id":"E","word_rank":2,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":4,"asc_desc_rank":4,"title":"hello kitty","description":"also known by her full name kitty white is a fictional character produced by the japanese company sanrio","tag":"green","":""} +{"id":"F","word_rank":2,"typo_rank":1,"proximity_rank":0,"attribute_rank":1017,"exact_rank":5,"asc_desc_rank":5,"title":"laptop orchestra","description":"a laptop orchestra lork or lo is a chamber music ensemble consisting primarily of laptops like helo huddersfield experimental laptop orchestra","tag":"blue","":""} +{"id":"G","word_rank":1,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":3,"asc_desc_rank":5,"title":"hello world film","description":"hello world is a 2019 japanese animated sci fi romantic drama film directed by tomohiko ito and produced by graphinica","tag":"red","":""} +{"id":"H","word_rank":1,"typo_rank":0,"proximity_rank":1,"attribute_rank":0,"exact_rank":3,"asc_desc_rank":4,"title":"world hello day","description":"holiday observed on november 21 to express that conflicts should be resolved through communication rather than the use of force","tag":"green","":""} +{"id":"I","word_rank":0,"typo_rank":0,"proximity_rank":8,"attribute_rank":338,"exact_rank":3,"asc_desc_rank":3,"title":"hello world song","description":"hello world is a song written by tom douglas tony lane and david lee and recorded by american country music group lady antebellum","tag":"blue","":""} +{"id":"J","word_rank":1,"typo_rank":0,"proximity_rank":1,"attribute_rank":1,"exact_rank":3,"asc_desc_rank":2,"title":"hello cruel world","description":"hello cruel world is an album by new zealand band tall dwarfs","tag":"green","":""} +{"id":"K","word_rank":0,"typo_rank":2,"proximity_rank":9,"attribute_rank":670,"exact_rank":5,"asc_desc_rank":1,"title":"ello creation system","description":"in few word ello was a construction toy created by the american company mattel to engage girls in construction play","tag":"red","":""} +{"id":"L","word_rank":0,"typo_rank":0,"proximity_rank":2,"attribute_rank":250,"exact_rank":4,"asc_desc_rank":0,"title":"good morning world","description":"good morning world is an american sitcom broadcast on cbs tv during the 1967 1968 season","tag":"blue","":""} +{"id":"M","word_rank":0,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":0,"asc_desc_rank":0,"title":"hello world america","description":"a perfect match for a perfect engine using the query hello world america","tag":"red","":""} +{"id":"N","word_rank":0,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":1,"asc_desc_rank":4,"title":"hello world america unleashed","description":"a very good match for a very good engine using the query hello world america","tag":"green","":""} +{"id":"O","word_rank":0,"typo_rank":0,"proximity_rank":0,"attribute_rank":10,"exact_rank":0,"asc_desc_rank":6,"title":"a perfect match for a perfect engine using the query hello world america","description":"hello world america","tag":"blue","":""} +{"id":"P","word_rank":0,"typo_rank":0,"proximity_rank":0,"attribute_rank":12,"exact_rank":1,"asc_desc_rank":3,"title":"a very good match for a very good engine using the query hello world america","description":"hello world america unleashed","tag":"red","":""} +{"id":"Q","word_rank":1,"typo_rank":0,"proximity_rank":0,"attribute_rank":0,"exact_rank":3,"asc_desc_rank":2,"title":"hello world","description":"a hello world program generally is a computer program that outputs or displays the message hello world","tag":"green","":""} diff --git a/milli/tests/search/distinct.rs b/milli/tests/search/distinct.rs index f044756eb..ef5af3272 100644 --- a/milli/tests/search/distinct.rs +++ b/milli/tests/search/distinct.rs @@ -32,7 +32,7 @@ macro_rules! test_distinct { let SearchResult { documents_ids, .. } = search.execute().unwrap(); let mut distinct_values = HashSet::new(); - let expected_external_ids: Vec<_> = search::expected_order(&criteria, true, true, &[]) + let expected_external_ids: Vec<_> = search::expected_order(&criteria, true, true) .into_iter() .filter_map(|d| { if distinct_values.contains(&d.$distinct) { diff --git a/milli/tests/search/filters.rs b/milli/tests/search/filters.rs index c810b47af..318197ea3 100644 --- a/milli/tests/search/filters.rs +++ b/milli/tests/search/filters.rs @@ -29,7 +29,7 @@ macro_rules! test_filter { let SearchResult { documents_ids, .. } = search.execute().unwrap(); let filtered_ids = search::expected_filtered_ids($filter); - let expected_external_ids: Vec<_> = search::expected_order(&criteria, true, true, &[]) + let expected_external_ids: Vec<_> = search::expected_order(&criteria, true, true) .into_iter() .filter_map(|d| if filtered_ids.contains(&d.id) { Some(d.id) } else { None }) .collect(); diff --git a/milli/tests/search/mod.rs b/milli/tests/search/mod.rs index 7d4043ff1..c5724a921 100644 --- a/milli/tests/search/mod.rs +++ b/milli/tests/search/mod.rs @@ -1,4 +1,3 @@ -use std::cmp::Reverse; use std::collections::HashSet; use big_s::S; @@ -6,7 +5,7 @@ use either::{Either, Left, Right}; use heed::EnvOpenOptions; use maplit::{hashmap, hashset}; use milli::update::{IndexDocuments, Settings, UpdateFormat}; -use milli::{AscDesc, Criterion, DocumentId, Index}; +use milli::{Criterion, DocumentId, Index}; use serde::Deserialize; use slice_group_by::GroupBy; @@ -37,10 +36,6 @@ pub fn setup_search_index_with_criteria(criteria: &[Criterion]) -> Index { S("tag"), S("asc_desc_rank"), }); - builder.set_sortable_fields(hashset! { - S("tag"), - S("asc_desc_rank"), - }); builder.set_synonyms(hashmap! { S("hello") => vec![S("good morning")], S("world") => vec![S("earth")], @@ -72,7 +67,6 @@ pub fn expected_order( criteria: &[Criterion], authorize_typo: bool, optional_words: bool, - sort_by: &[AscDesc], ) -> Vec { let dataset = serde_json::Deserializer::from_str(CONTENT).into_iter().map(|r| r.unwrap()).collect(); @@ -96,14 +90,6 @@ pub fn expected_order( new_groups .extend(group.linear_group_by_key(|d| d.proximity_rank).map(Vec::from)); } - Criterion::Sort if sort_by == [AscDesc::Asc(S("tag"))] => { - group.sort_by_key(|d| d.sort_by_rank); - new_groups.extend(group.linear_group_by_key(|d| d.sort_by_rank).map(Vec::from)); - } - Criterion::Sort if sort_by == [AscDesc::Desc(S("tag"))] => { - group.sort_by_key(|d| Reverse(d.sort_by_rank)); - new_groups.extend(group.linear_group_by_key(|d| d.sort_by_rank).map(Vec::from)); - } Criterion::Typo => { group.sort_by_key(|d| d.typo_rank); new_groups.extend(group.linear_group_by_key(|d| d.typo_rank).map(Vec::from)); @@ -118,13 +104,11 @@ pub fn expected_order( .extend(group.linear_group_by_key(|d| d.asc_desc_rank).map(Vec::from)); } Criterion::Desc(field_name) if field_name == "asc_desc_rank" => { - group.sort_by_key(|d| Reverse(d.asc_desc_rank)); + group.sort_by_key(|d| std::cmp::Reverse(d.asc_desc_rank)); new_groups .extend(group.linear_group_by_key(|d| d.asc_desc_rank).map(Vec::from)); } - Criterion::Asc(_) | Criterion::Desc(_) | Criterion::Sort => { - new_groups.push(group.clone()) - } + Criterion::Asc(_) | Criterion::Desc(_) => new_groups.push(group.clone()), } } groups = std::mem::take(&mut new_groups); @@ -201,7 +185,6 @@ pub struct TestDocument { pub attribute_rank: u32, pub exact_rank: u32, pub asc_desc_rank: u32, - pub sort_by_rank: u32, pub title: String, pub description: String, pub tag: String, diff --git a/milli/tests/search/query_criteria.rs b/milli/tests/search/query_criteria.rs index 1723c1d6f..f814508f5 100644 --- a/milli/tests/search/query_criteria.rs +++ b/milli/tests/search/query_criteria.rs @@ -1,6 +1,6 @@ use big_s::S; use milli::update::Settings; -use milli::{AscDesc, Criterion, Search, SearchResult}; +use milli::{Criterion, Search, SearchResult}; use Criterion::*; use crate::search::{self, EXTERNAL_DOCUMENTS_IDS}; @@ -11,7 +11,7 @@ const ALLOW_OPTIONAL_WORDS: bool = true; const DISALLOW_OPTIONAL_WORDS: bool = false; macro_rules! test_criterion { - ($func:ident, $optional_word:ident, $authorize_typos:ident, $criteria:expr, $sort_criteria:expr) => { + ($func:ident, $optional_word:ident, $authorize_typos:ident, $criteria:expr) => { #[test] fn $func() { let criteria = $criteria; @@ -23,168 +23,82 @@ macro_rules! test_criterion { search.limit(EXTERNAL_DOCUMENTS_IDS.len()); search.authorize_typos($authorize_typos); search.optional_words($optional_word); - search.sort_criteria($sort_criteria); let SearchResult { documents_ids, .. } = search.execute().unwrap(); - let expected_external_ids: Vec<_> = search::expected_order( - &criteria, - $authorize_typos, - $optional_word, - &$sort_criteria[..], - ) - .into_iter() - .map(|d| d.id) - .collect(); + let expected_external_ids: Vec<_> = + search::expected_order(&criteria, $authorize_typos, $optional_word) + .into_iter() + .map(|d| d.id) + .collect(); let documents_ids = search::internal_to_external_ids(&index, &documents_ids); assert_eq!(documents_ids, expected_external_ids); } }; } -test_criterion!(none_allow_typo, ALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, vec![], vec![]); -test_criterion!(none_disallow_typo, DISALLOW_OPTIONAL_WORDS, DISALLOW_TYPOS, vec![], vec![]); -test_criterion!(words_allow_typo, ALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, vec![Words], vec![]); -test_criterion!( - attribute_allow_typo, - DISALLOW_OPTIONAL_WORDS, - ALLOW_TYPOS, - vec![Attribute], - vec![] -); -test_criterion!( - attribute_disallow_typo, - DISALLOW_OPTIONAL_WORDS, - DISALLOW_TYPOS, - vec![Attribute], - vec![] -); -test_criterion!( - exactness_allow_typo, - DISALLOW_OPTIONAL_WORDS, - ALLOW_TYPOS, - vec![Exactness], - vec![] -); -test_criterion!( - exactness_disallow_typo, - DISALLOW_OPTIONAL_WORDS, - DISALLOW_TYPOS, - vec![Exactness], - vec![] -); -test_criterion!( - proximity_allow_typo, - DISALLOW_OPTIONAL_WORDS, - ALLOW_TYPOS, - vec![Proximity], - vec![] -); -test_criterion!( - proximity_disallow_typo, - DISALLOW_OPTIONAL_WORDS, - DISALLOW_TYPOS, - vec![Proximity], - vec![] -); +test_criterion!(none_allow_typo, ALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, vec![]); +test_criterion!(none_disallow_typo, DISALLOW_OPTIONAL_WORDS, DISALLOW_TYPOS, vec![]); +test_criterion!(words_allow_typo, ALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, vec![Words]); +test_criterion!(attribute_allow_typo, DISALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, vec![Attribute]); +test_criterion!(attribute_disallow_typo, DISALLOW_OPTIONAL_WORDS, DISALLOW_TYPOS, vec![Attribute]); +test_criterion!(exactness_allow_typo, DISALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, vec![Exactness]); +test_criterion!(exactness_disallow_typo, DISALLOW_OPTIONAL_WORDS, DISALLOW_TYPOS, vec![Exactness]); +test_criterion!(proximity_allow_typo, DISALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, vec![Proximity]); +test_criterion!(proximity_disallow_typo, DISALLOW_OPTIONAL_WORDS, DISALLOW_TYPOS, vec![Proximity]); test_criterion!( asc_allow_typo, DISALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, - vec![Asc(S("asc_desc_rank"))], - vec![] + vec![Asc(S("asc_desc_rank"))] ); test_criterion!( asc_disallow_typo, DISALLOW_OPTIONAL_WORDS, DISALLOW_TYPOS, - vec![Asc(S("asc_desc_rank"))], - vec![] + vec![Asc(S("asc_desc_rank"))] ); test_criterion!( desc_allow_typo, DISALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, - vec![Desc(S("asc_desc_rank"))], - vec![] + vec![Desc(S("asc_desc_rank"))] ); test_criterion!( desc_disallow_typo, DISALLOW_OPTIONAL_WORDS, DISALLOW_TYPOS, - vec![Desc(S("asc_desc_rank"))], - vec![] + vec![Desc(S("asc_desc_rank"))] ); test_criterion!( asc_unexisting_field_allow_typo, DISALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, - vec![Asc(S("unexisting_field"))], - vec![] + vec![Asc(S("unexisting_field"))] ); test_criterion!( asc_unexisting_field_disallow_typo, DISALLOW_OPTIONAL_WORDS, DISALLOW_TYPOS, - vec![Asc(S("unexisting_field"))], - vec![] + vec![Asc(S("unexisting_field"))] ); test_criterion!( desc_unexisting_field_allow_typo, DISALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, - vec![Desc(S("unexisting_field"))], - vec![] + vec![Desc(S("unexisting_field"))] ); test_criterion!( desc_unexisting_field_disallow_typo, DISALLOW_OPTIONAL_WORDS, DISALLOW_TYPOS, - vec![Desc(S("unexisting_field"))], - vec![] -); -test_criterion!(empty_sort_by_allow_typo, DISALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, vec![Sort], vec![]); -test_criterion!( - empty_sort_by_disallow_typo, - DISALLOW_OPTIONAL_WORDS, - DISALLOW_TYPOS, - vec![Sort], - vec![] -); -test_criterion!( - sort_by_asc_allow_typo, - DISALLOW_OPTIONAL_WORDS, - ALLOW_TYPOS, - vec![Sort], - vec![AscDesc::Asc(S("tag"))] -); -test_criterion!( - sort_by_asc_disallow_typo, - DISALLOW_OPTIONAL_WORDS, - DISALLOW_TYPOS, - vec![Sort], - vec![AscDesc::Asc(S("tag"))] -); -test_criterion!( - sort_by_desc_allow_typo, - DISALLOW_OPTIONAL_WORDS, - ALLOW_TYPOS, - vec![Sort], - vec![AscDesc::Desc(S("tag"))] -); -test_criterion!( - sort_by_desc_disallow_typo, - DISALLOW_OPTIONAL_WORDS, - DISALLOW_TYPOS, - vec![Sort], - vec![AscDesc::Desc(S("tag"))] + vec![Desc(S("unexisting_field"))] ); test_criterion!( default_criteria_order, ALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, - vec![Words, Typo, Proximity, Attribute, Exactness], - vec![] + vec![Words, Typo, Proximity, Attribute, Exactness] ); #[test] @@ -348,7 +262,7 @@ fn criteria_mixup() { let SearchResult { documents_ids, .. } = search.execute().unwrap(); let expected_external_ids: Vec<_> = - search::expected_order(&criteria, ALLOW_OPTIONAL_WORDS, ALLOW_TYPOS, &[]) + search::expected_order(&criteria, ALLOW_OPTIONAL_WORDS, ALLOW_TYPOS) .into_iter() .map(|d| d.id) .collect();