From effbb7f7f1e31bdc46155ecfeca0d7dd0417aaa1 Mon Sep 17 00:00:00 2001 From: mpostma Date: Thu, 7 May 2020 19:25:18 +0200 Subject: [PATCH 1/8] add sort result struct --- Cargo.lock | 92 +++++++++++---------- meilisearch-core/src/bucket_sort.rs | 59 +++++++++++-- meilisearch-core/src/query_builder.rs | 47 +++++++++-- meilisearch-core/src/store/facets.rs | 7 +- meilisearch-core/src/store/mod.rs | 4 +- meilisearch-http/src/helpers/meilisearch.rs | 19 +++-- meilisearch-http/src/routes/search.rs | 35 ++++++++ 7 files changed, 194 insertions(+), 69 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 26b4bb72c..61c7cd4f5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -522,9 +522,9 @@ dependencies = [ [[package]] name = "bstr" -version = "0.2.12" +version = "0.2.13" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2889e6d50f394968c8bf4240dc3f2a7eb4680844d27308f798229ac9d4725f41" +checksum = "31accafdb70df7871592c058eca3985b71104e15ac32f64706022c58867da931" dependencies = [ "lazy_static", "memchr", @@ -616,9 +616,9 @@ checksum = "5b89647f09b9f4c838cb622799b2843e4e13bff64661dab9a0362bb92985addd" [[package]] name = "clap" -version = "2.33.0" +version = "2.33.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5067f5bb2d80ef5d68b4c87db81601f0b75bca627bc2ef76b141d7b846a3c6d9" +checksum = "bdfa80d47f954d53a35a64987ca1422f495b8d6483c0fe9f7117b36c2a792129" dependencies = [ "ansi_term", "atty", @@ -1008,9 +1008,9 @@ checksum = "3dcaa9ae7725d12cdb85b3ad99a434db70b468c09ded17e012d86b5c1010f7a7" [[package]] name = "futures" -version = "0.3.4" +version = "0.3.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5c329ae8753502fb44ae4fc2b622fa2a94652c41e795143765ba0927f92ab780" +checksum = "1e05b85ec287aac0dc34db7d4a569323df697f9c55b99b15d6b4ef8cde49f613" dependencies = [ "futures-channel", "futures-core", @@ -1023,9 +1023,9 @@ dependencies = [ [[package]] name = "futures-channel" -version = "0.3.4" +version = "0.3.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f0c77d04ce8edd9cb903932b608268b3fffec4163dc053b3b402bf47eac1f1a8" +checksum = "f366ad74c28cca6ba456d95e6422883cfb4b252a83bed929c83abfdbbf2967d5" dependencies = [ "futures-core", "futures-sink", @@ -1033,15 +1033,15 @@ dependencies = [ [[package]] name = "futures-core" -version = "0.3.4" +version = "0.3.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f25592f769825e89b92358db00d26f965761e094951ac44d3663ef25b7ac464a" +checksum = "59f5fff90fd5d971f936ad674802482ba441b6f09ba5e15fd8b39145582ca399" [[package]] name = "futures-executor" -version = "0.3.4" +version = "0.3.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f674f3e1bcb15b37284a90cedf55afdba482ab061c407a9c0ebbd0f3109741ba" +checksum = "10d6bb888be1153d3abeb9006b11b02cf5e9b209fda28693c31ae1e4e012e314" dependencies = [ "futures-core", "futures-task", @@ -1050,15 +1050,15 @@ dependencies = [ [[package]] name = "futures-io" -version = "0.3.4" +version = "0.3.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a638959aa96152c7a4cddf50fcb1e3fede0583b27157c26e67d6f99904090dc6" +checksum = "de27142b013a8e869c14957e6d2edeef89e97c289e69d042ee3a49acd8b51789" [[package]] name = "futures-macro" -version = "0.3.4" +version = "0.3.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9a5081aa3de1f7542a794a397cde100ed903b0630152d0973479018fd85423a7" +checksum = "d0b5a30a4328ab5473878237c447333c093297bded83a4983d10f4deea240d39" dependencies = [ "proc-macro-hack", "proc-macro2", @@ -1068,21 +1068,24 @@ dependencies = [ [[package]] name = "futures-sink" -version = "0.3.4" +version = "0.3.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3466821b4bc114d95b087b850a724c6f83115e929bc88f1fa98a3304a944c8a6" +checksum = "3f2032893cb734c7a05d85ce0cc8b8c4075278e93b24b66f9de99d6eb0fa8acc" [[package]] name = "futures-task" -version = "0.3.4" +version = "0.3.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7b0a34e53cf6cdcd0178aa573aed466b646eb3db769570841fda0c7ede375a27" +checksum = "bdb66b5f09e22019b1ab0830f7785bcea8e7a42148683f99214f73f8ec21a626" +dependencies = [ + "once_cell", +] [[package]] name = "futures-util" -version = "0.3.4" +version = "0.3.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22766cf25d64306bedf0384da004d05c9974ab104fcc4528f1236181c18004c5" +checksum = "8764574ff08b701a084482c3c7031349104b07ac897393010494beaa18ce32c6" dependencies = [ "futures-channel", "futures-core", @@ -1091,6 +1094,7 @@ dependencies = [ "futures-sink", "futures-task", "memchr", + "pin-project", "pin-utils", "proc-macro-hack", "proc-macro-nested", @@ -1398,9 +1402,9 @@ dependencies = [ [[package]] name = "intervaltree" -version = "0.2.5" +version = "0.2.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8254add2ea664734c9d001f8151cc3d7696b135f7e40e5a2efa814a662cb3a44" +checksum = "566d5aa3b5cc5c5809cc1a9c9588d917a634248bfc58f7ea14e354e71595a32c" dependencies = [ "smallvec", ] @@ -1842,9 +1846,9 @@ dependencies = [ [[package]] name = "ntapi" -version = "0.3.3" +version = "0.3.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f26e041cd983acbc087e30fcba770380cfa352d0e392e175b2344ebaf7ea0602" +checksum = "7a31937dea023539c72ddae0e3571deadc1414b300483fa7aaec176168cfa9d2" dependencies = [ "winapi 0.3.8", ] @@ -1918,9 +1922,9 @@ checksum = "77af24da69f9d9341038eba93a073b1fdaaa1b788221b00a69bce9e762cb32de" [[package]] name = "openssl-sys" -version = "0.9.55" +version = "0.9.56" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7717097d810a0f2e2323f9e5d11e71608355e24828410b55b9d4f18aa5f9a5d8" +checksum = "f02309a7f127000ed50594f0b50ecc69e7c654e16d41b4e8156d1b3df8e0b52e" dependencies = [ "autocfg", "cc", @@ -2038,18 +2042,18 @@ dependencies = [ [[package]] name = "pin-project" -version = "0.4.13" +version = "0.4.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "82c3bfbfb5bb42f99498c7234bbd768c220eb0cea6818259d0d18a1aa3d2595d" +checksum = "81d480cb4e89522ccda96d0eed9af94180b7a5f93fb28f66e1fd7d68431663d1" dependencies = [ "pin-project-internal", ] [[package]] name = "pin-project-internal" -version = "0.4.13" +version = "0.4.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ccbf6449dcfb18562c015526b085b8df1aa3cdab180af8ec2ebd300a3bd28f63" +checksum = "a82996f11efccb19b685b14b5df818de31c1edcee3daa256ab5775dd98e72feb" dependencies = [ "proc-macro2", "quote", @@ -2466,9 +2470,9 @@ dependencies = [ [[package]] name = "schannel" -version = "0.1.18" +version = "0.1.19" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "039c25b130bd8c1321ee2d7de7fde2659fa9c2744e4bb29711cfc852ea53cd19" +checksum = "8f05ba609c234e60bee0d547fe94a4c7e9da733d1c962cf6e59efa4cd9c8bc75" dependencies = [ "lazy_static", "winapi 0.3.8", @@ -2498,9 +2502,9 @@ checksum = "cbb21fe0588557792176c89bc7b943027b14f346d03c6be6a199c2860277d93a" [[package]] name = "security-framework" -version = "0.4.3" +version = "0.4.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3f331b9025654145cd425b9ded0caf8f5ae0df80d418b326e2dc1c3dc5eb0620" +checksum = "64808902d7d99f78eaddd2b4e2509713babc3dc3c85ad6f4c447680f3c01e535" dependencies = [ "bitflags", "core-foundation", @@ -2575,18 +2579,18 @@ dependencies = [ [[package]] name = "serde" -version = "1.0.106" +version = "1.0.110" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "36df6ac6412072f67cf767ebbde4133a5b2e88e76dc6187fa7104cd16f783399" +checksum = "99e7b308464d16b56eba9964e4972a3eee817760ab60d88c3f86e1fecb08204c" dependencies = [ "serde_derive", ] [[package]] name = "serde_derive" -version = "1.0.106" +version = "1.0.110" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9e549e3abf4fb8621bd1609f11dfc9f5e50320802273b12f3811a67e6716ea6c" +checksum = "818fbf6bfa9a42d3bfcaca148547aa00c7b915bec71d1757aa2d44ca68771984" dependencies = [ "proc-macro2", "quote", @@ -2595,9 +2599,9 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.52" +version = "1.0.53" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a7894c8ed05b7a3a279aeb79025fdec1d3158080b75b98a08faf2806bb799edd" +checksum = "993948e75b189211a9b31a7528f950c6adc21f9720b6438ff80a7fa2f864cea2" dependencies = [ "indexmap", "itoa", @@ -2853,9 +2857,9 @@ dependencies = [ [[package]] name = "threadpool" -version = "1.8.0" +version = "1.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e8dae184447c15d5a6916d973c642aec485105a13cd238192a6927ae3e077d66" +checksum = "d050e60b33d41c19108b32cea32164033a9013fe3b46cbd4457559bfbf77afaa" dependencies = [ "num_cpus", ] diff --git a/meilisearch-core/src/bucket_sort.rs b/meilisearch-core/src/bucket_sort.rs index ec45f36d0..8f391d74c 100644 --- a/meilisearch-core/src/bucket_sort.rs +++ b/meilisearch-core/src/bucket_sort.rs @@ -11,12 +11,13 @@ use std::fmt; use compact_arena::{SmallArena, Idx32, mk_arena}; use log::debug; use meilisearch_types::DocIndex; -use sdset::{Set, SetBuf, exponential_search, SetOperation}; +use sdset::{Set, SetBuf, exponential_search, SetOperation, Counter, duo::OpBuilder}; use slice_group_by::{GroupBy, GroupByMut}; use crate::error::Error; use crate::criterion::{Criteria, Context, ContextMut}; use crate::distinct_map::{BufferedDistinctMap, DistinctMap}; +use crate::facets::FacetKey; use crate::raw_document::RawDocument; use crate::{database::MainT, reordered_attrs::ReorderedAttrs}; use crate::{store, Document, DocumentId, MResult}; @@ -24,11 +25,20 @@ use crate::query_tree::{create_query_tree, traverse_query_tree}; use crate::query_tree::{Operation, QueryResult, QueryKind, QueryId, PostingsKey}; use crate::query_tree::Context as QTContext; +#[derive(Debug, Default)] +pub struct SortResult { + pub documents: Vec, + pub nb_hits: usize, + pub is_exhaustive: bool, + pub facets: Option>, +} + pub fn bucket_sort<'c, FI>( reader: &heed::RoTxn, query: &str, range: Range, facets_docids: Option>, + facet_count_docids: Option>>>, filter: Option, criteria: Criteria<'c>, searchable_attrs: Option, @@ -38,7 +48,7 @@ pub fn bucket_sort<'c, FI>( synonyms_store: store::Synonyms, prefix_documents_cache_store: store::PrefixDocumentsCache, prefix_postings_lists_cache_store: store::PrefixPostingsListsCache, -) -> MResult<(Vec, usize)> +) -> MResult where FI: Fn(DocumentId) -> bool, { @@ -52,6 +62,7 @@ where query, range, facets_docids, + facet_count_docids, filter, distinct, distinct_size, @@ -66,9 +77,11 @@ where ); } + let mut result = SortResult::default(); + let words_set = match unsafe { main_store.static_words_fst(reader)? } { Some(words) => words, - None => return Ok((Vec::new(), 0)), + None => return Ok(SortResult::default()), }; let stop_words = main_store.stop_words_fst(reader)?.unwrap_or_default(); @@ -107,6 +120,17 @@ where docids = Cow::Owned(intersection); } + if let Some(facet_count_docids) = facet_count_docids { + let mut facets = HashMap::new(); + for (key, document_ids) in facet_count_docids { + let mut counter = Counter::new(); + let op = OpBuilder::new(document_ids.as_ref(), document_ids.as_ref()).intersection(); + SetOperation::::extend_collection(op, &mut counter); + facets.insert(key, counter.0); + } + result.facets = Some(facets); + } + let before = Instant::now(); mk_arena!(arena); let mut bare_matches = cleanup_bare_matches(&mut arena, &docids, queries); @@ -181,7 +205,10 @@ where debug!("bucket sort took {:.02?}", before_bucket_sort.elapsed()); - Ok((documents, docids.len())) + result.documents = documents; + result.nb_hits = docids.len(); + + Ok(result) } pub fn bucket_sort_with_distinct<'c, FI, FD>( @@ -189,6 +216,7 @@ pub fn bucket_sort_with_distinct<'c, FI, FD>( query: &str, range: Range, facets_docids: Option>, + facet_count_docids: Option>>>, filter: Option, distinct: FD, distinct_size: usize, @@ -200,14 +228,16 @@ pub fn bucket_sort_with_distinct<'c, FI, FD>( synonyms_store: store::Synonyms, _prefix_documents_cache_store: store::PrefixDocumentsCache, prefix_postings_lists_cache_store: store::PrefixPostingsListsCache, -) -> MResult<(Vec, usize)> +) -> MResult where FI: Fn(DocumentId) -> bool, FD: Fn(DocumentId) -> Option, { + let mut result = SortResult::default(); + let words_set = match unsafe { main_store.static_words_fst(reader)? } { Some(words) => words, - None => return Ok((Vec::new(), 0)), + None => return Ok(SortResult::default()), }; let stop_words = main_store.stop_words_fst(reader)?.unwrap_or_default(); @@ -240,12 +270,23 @@ where debug!("number of postings {:?}", queries.len()); if let Some(facets_docids) = facets_docids { - let intersection = sdset::duo::OpBuilder::new(docids.as_ref(), facets_docids.as_set()) + let intersection = OpBuilder::new(docids.as_ref(), facets_docids.as_set()) .intersection() .into_set_buf(); docids = Cow::Owned(intersection); } + if let Some(facet_count_docids) = facet_count_docids { + let mut facets = HashMap::new(); + for (key, document_ids) in facet_count_docids { + let mut counter = Counter::new(); + let op = OpBuilder::new(document_ids.as_ref(), document_ids.as_ref()).intersection(); + SetOperation::::extend_collection(op, &mut counter); + facets.insert(key, counter.0); + } + result.facets = Some(facets); + } + let before = Instant::now(); mk_arena!(arena); let mut bare_matches = cleanup_bare_matches(&mut arena, &docids, queries); @@ -379,8 +420,10 @@ where } } } + result.documents = documents; + result.nb_hits = docids.len(); - Ok((documents, docids.len())) + Ok(result) } fn cleanup_bare_matches<'tag, 'txn>( diff --git a/meilisearch-core/src/query_builder.rs b/meilisearch-core/src/query_builder.rs index b3bbf21bf..77c0421d6 100644 --- a/meilisearch-core/src/query_builder.rs +++ b/meilisearch-core/src/query_builder.rs @@ -1,27 +1,31 @@ use std::borrow::Cow; +use std::collections::HashMap; use std::ops::{Range, Deref}; use std::time::Duration; use crate::database::MainT; -use crate::bucket_sort::{bucket_sort, bucket_sort_with_distinct}; -use crate::{criterion::Criteria, Document, DocumentId}; +use crate::bucket_sort::{bucket_sort, bucket_sort_with_distinct, SortResult}; +use crate::{criterion::Criteria, DocumentId}; use crate::{reordered_attrs::ReorderedAttrs, store, MResult}; use crate::facets::FacetFilter; use either::Either; use sdset::SetOperation; -pub struct QueryBuilder<'c, 'f, 'd, 'fa, 'i> { +use meilisearch_schema::FieldId; + +pub struct QueryBuilder<'c, 'f, 'd, 'i, 'q> { criteria: Criteria<'c>, searchable_attrs: Option, filter: Option bool + 'f>>, distinct: Option<(Box Option + 'd>, usize)>, timeout: Option, index: &'i store::Index, - facets: Option<&'fa FacetFilter>, + facet_fitlers: Option<&'q FacetFilter>, + facets: Option<&'q [FieldId]>, } -impl<'c, 'f, 'd, 'fa, 'i> QueryBuilder<'c, 'f, 'd, 'fa, 'i> { +impl<'c, 'f, 'd, 'i, 'q> QueryBuilder<'c, 'f, 'd, 'i, 'q> { pub fn new(index: &'i store::Index) -> Self { QueryBuilder::with_criteria( index, @@ -29,7 +33,13 @@ impl<'c, 'f, 'd, 'fa, 'i> QueryBuilder<'c, 'f, 'd, 'fa, 'i> { ) } - pub fn set_facets(&mut self, facets: Option<&'fa FacetFilter>) { + /// sets facet attributes to filter on + pub fn set_facet_filters(&mut self, facets: Option<&'q FacetFilter>) { + self.facet_fitlers = facets; + } + + /// sets facet attributes for which to return the count + pub fn set_facets(&mut self, facets: Option<&'q [FieldId]>) { self.facets = facets; } @@ -44,6 +54,7 @@ impl<'c, 'f, 'd, 'fa, 'i> QueryBuilder<'c, 'f, 'd, 'fa, 'i> { distinct: None, timeout: None, index, + facet_fitlers: None, facets: None, } } @@ -76,8 +87,8 @@ impl<'c, 'f, 'd, 'fa, 'i> QueryBuilder<'c, 'f, 'd, 'fa, 'i> { reader: &heed::RoTxn, query: &str, range: Range, - ) -> MResult<(Vec, usize)> { - let facets_docids = match self.facets { + ) -> MResult { + let facets_docids = match self.facet_fitlers { Some(facets) => { let mut ands = Vec::with_capacity(facets.len()); let mut ors = Vec::new(); @@ -98,7 +109,7 @@ impl<'c, 'f, 'd, 'fa, 'i> QueryBuilder<'c, 'f, 'd, 'fa, 'i> { match self.index.facets.facet_document_ids(reader, &key)? { Some(docids) => ands.push(docids), // no candidates for search, early return. - None => return Ok((vec![], 0)), + None => return Ok(SortResult::default()), } } }; @@ -109,12 +120,29 @@ impl<'c, 'f, 'd, 'fa, 'i> QueryBuilder<'c, 'f, 'd, 'fa, 'i> { None => None }; + let facet_count_docids = match self.facets { + Some(field_ids) => { + let mut facet_count_map = HashMap::new(); + for field_id in field_ids { + for pair in self.index.facets.field_document_ids(reader, *field_id)? { + let (facet_key, document_ids) = pair?; + let facet_key_string = facet_key.to_parts(schema)?; + facet_count_map.insert(facet_key, document_ids); + } + } + Some(facet_count_map) + } + None => None, + }; + + match self.distinct { Some((distinct, distinct_size)) => bucket_sort_with_distinct( reader, query, range, facets_docids, + facet_count_docids, self.filter, distinct, distinct_size, @@ -132,6 +160,7 @@ impl<'c, 'f, 'd, 'fa, 'i> QueryBuilder<'c, 'f, 'd, 'fa, 'i> { query, range, facets_docids, + facet_count_docids, self.filter, self.criteria, self.searchable_attrs, diff --git a/meilisearch-core/src/store/facets.rs b/meilisearch-core/src/store/facets.rs index c9811f765..907ceae9b 100644 --- a/meilisearch-core/src/store/facets.rs +++ b/meilisearch-core/src/store/facets.rs @@ -1,10 +1,11 @@ use std::borrow::Cow; use std::collections::HashMap; -use heed::{RwTxn, RoTxn, Result as ZResult}; +use heed::{RwTxn, RoTxn, Result as ZResult, RoRange}; use sdset::{SetBuf, Set, SetOperation}; use meilisearch_types::DocumentId; +use meilisearch_schema::FieldId; use crate::database::MainT; use crate::facets::FacetKey; @@ -22,6 +23,10 @@ impl Facets { self.facets.put(writer, &facet_key, doc_ids) } + pub fn field_document_ids<'txn>(&self, reader: &'txn RoTxn, field_id: FieldId) -> ZResult>> { + self.facets.prefix_iter(reader, &FacetKey::new(field_id, "".to_string())) + } + pub fn facet_document_ids<'txn>(&self, reader: &'txn RoTxn, facet_key: &FacetKey) -> ZResult>>> { self.facets.get(reader, &facet_key) } diff --git a/meilisearch-core/src/store/mod.rs b/meilisearch-core/src/store/mod.rs index c0df09809..ddf6614f5 100644 --- a/meilisearch-core/src/store/mod.rs +++ b/meilisearch-core/src/store/mod.rs @@ -363,10 +363,10 @@ impl Index { QueryBuilder::new(self) } - pub fn query_builder_with_criteria<'c, 'f, 'd, 'fa, 'i>( + pub fn query_builder_with_criteria<'c, 'f, 'd, 'fa, 'i, 'q>( &'i self, criteria: Criteria<'c>, - ) -> QueryBuilder<'c, 'f, 'd, 'fa, 'i> { + ) -> QueryBuilder<'c, 'f, 'd, 'i, 'q> { QueryBuilder::with_criteria(self, criteria) } } diff --git a/meilisearch-http/src/helpers/meilisearch.rs b/meilisearch-http/src/helpers/meilisearch.rs index 94f496c07..13fdea2d4 100644 --- a/meilisearch-http/src/helpers/meilisearch.rs +++ b/meilisearch-http/src/helpers/meilisearch.rs @@ -36,6 +36,7 @@ impl IndexSearchExt for Index { filters: None, matches: false, facet_filters: None, + facets: None, } } } @@ -51,6 +52,7 @@ pub struct SearchBuilder<'a> { filters: Option, matches: bool, facet_filters: Option, + facets: Option> } impl<'a> SearchBuilder<'a> { @@ -100,6 +102,11 @@ impl<'a> SearchBuilder<'a> { self } + pub fn add_facets(&mut self, facets: Vec) -> &SearchBuilder { + self.facets = Some(facets); + self + } + pub fn search(&self, reader: &heed::RoTxn) -> Result { let schema = self .index @@ -146,11 +153,12 @@ impl<'a> SearchBuilder<'a> { } } - query_builder.set_facets(self.facet_filters.as_ref()); + query_builder.set_facet_filters(self.facet_filters.as_ref()); + query_builder.set_facets(self.facets.as_deref()); let start = Instant::now(); let result = query_builder.query(reader, &self.query, self.offset..(self.offset + self.limit)); - let (docs, nb_hits) = result.map_err(ResponseError::search_documents)?; + let search_result = result.map_err(ResponseError::search_documents)?; let time_ms = start.elapsed().as_millis() as usize; let mut all_attributes: HashSet<&str> = HashSet::new(); @@ -181,7 +189,7 @@ impl<'a> SearchBuilder<'a> { } let mut hits = Vec::with_capacity(self.limit); - for doc in docs { + for doc in search_result.documents { let mut document: IndexMap = self .index .document(reader, Some(&all_attributes), doc.id) @@ -235,10 +243,11 @@ impl<'a> SearchBuilder<'a> { hits, offset: self.offset, limit: self.limit, - nb_hits, - exhaustive_nb_hits: false, + nb_hits: search_result.nb_hits, + exhaustive_nb_hits: search_result.is_exhaustive, processing_time_ms: time_ms, query: self.query.to_string(), + facets: search_result.facets }; Ok(results) diff --git a/meilisearch-http/src/routes/search.rs b/meilisearch-http/src/routes/search.rs index 846fc8904..0468784f5 100644 --- a/meilisearch-http/src/routes/search.rs +++ b/meilisearch-http/src/routes/search.rs @@ -5,6 +5,7 @@ use actix_web::web; use actix_web::HttpResponse; use actix_web_macros::get; use serde::Deserialize; +use serde_json::Value; use crate::error::ResponseError; use crate::helpers::meilisearch::IndexSearchExt; @@ -13,6 +14,7 @@ use crate::routes::IndexParam; use crate::Data; use meilisearch_core::facets::FacetFilter; +use meilisearch_schema::{Schema, FieldId}; pub fn services(cfg: &mut web::ServiceConfig) { cfg.service(search_with_url_query); @@ -31,6 +33,7 @@ struct SearchQuery { filters: Option, matches: Option, facet_filters: Option, + facets: Option } #[get("/indexes/{index_uid}/search", wrap = "Authentication::Public")] @@ -91,6 +94,14 @@ async fn search_with_url_query( } } + if let Some(ref facets) = params.facets { + match index.main.attributes_for_faceting(&reader)? { + Some(ref attrs) => { search_builder.add_facets(prepare_facet_list(facets, &schema, attrs)?); }, + None => return Err(ResponseError::FacetExpression("can't return facets count, as no facet is set".to_string())) + } + + } + if let Some(attributes_to_crop) = ¶ms.attributes_to_crop { let default_length = params.crop_length.unwrap_or(200); let mut final_attributes: HashMap = HashMap::new(); @@ -150,3 +161,27 @@ async fn search_with_url_query( Ok(HttpResponse::Ok().json(search_builder.search(&reader)?)) } + +fn prepare_facet_list<'fa>(facets: &str, schema: &Schema, facet_attrs: &'fa [FieldId]) -> Result, ResponseError> { + let facet_array = serde_json::from_str(facets).expect("do error handling"); // TODO + match facet_array { + Value::Array(facet_array) => { + let wild_card = Value::String("*".to_string()); + if facet_array.iter().any(|it| it == &wild_card) { + return Ok(Vec::from(facet_attrs)); // TODO can make cow? + } + let mut fields = Vec::with_capacity(facet_attrs.len()); + for v in facet_array { + match v { + Value::String(name) => { + let id = schema.id(&name).expect("not found error"); // TODO + fields.push(id); + } + _ => todo!("expected string, found {}", v), + } + } + return Ok(fields); + } + _ => todo!("error, bad syntax, expected array") + } +} From e5126af458c4c90628183b12ab92428022e98272 Mon Sep 17 00:00:00 2001 From: mpostma Date: Tue, 12 May 2020 11:22:09 +0200 Subject: [PATCH 2/8] enables facet count --- Cargo.lock | 2 +- meilisearch-core/src/bucket_sort.rs | 53 ++++++++++++--------- meilisearch-core/src/query_builder.rs | 30 +++++++----- meilisearch-core/src/store/facets.rs | 2 +- meilisearch-core/src/store/mod.rs | 2 +- meilisearch-http/Cargo.toml | 2 +- meilisearch-http/src/helpers/meilisearch.rs | 5 +- meilisearch-http/src/routes/search.rs | 50 +++++++++++-------- 8 files changed, 88 insertions(+), 58 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 61c7cd4f5..f56456fbc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1664,8 +1664,8 @@ dependencies = [ "mime", "pretty-bytes", "rand 0.7.3", - "sentry", "regex", + "sentry", "serde", "serde_json", "serde_qs", diff --git a/meilisearch-core/src/bucket_sort.rs b/meilisearch-core/src/bucket_sort.rs index 8f391d74c..337bd8cce 100644 --- a/meilisearch-core/src/bucket_sort.rs +++ b/meilisearch-core/src/bucket_sort.rs @@ -17,7 +17,6 @@ use slice_group_by::{GroupBy, GroupByMut}; use crate::error::Error; use crate::criterion::{Criteria, Context, ContextMut}; use crate::distinct_map::{BufferedDistinctMap, DistinctMap}; -use crate::facets::FacetKey; use crate::raw_document::RawDocument; use crate::{database::MainT, reordered_attrs::ReorderedAttrs}; use crate::{store, Document, DocumentId, MResult}; @@ -30,7 +29,8 @@ pub struct SortResult { pub documents: Vec, pub nb_hits: usize, pub is_exhaustive: bool, - pub facets: Option>, + pub facets: Option>>, + pub exhaustive_facet_count: Option, } pub fn bucket_sort<'c, FI>( @@ -38,7 +38,7 @@ pub fn bucket_sort<'c, FI>( query: &str, range: Range, facets_docids: Option>, - facet_count_docids: Option>>>, + facet_count_docids: Option>>>>, filter: Option, criteria: Criteria<'c>, searchable_attrs: Option, @@ -120,15 +120,10 @@ where docids = Cow::Owned(intersection); } - if let Some(facet_count_docids) = facet_count_docids { - let mut facets = HashMap::new(); - for (key, document_ids) in facet_count_docids { - let mut counter = Counter::new(); - let op = OpBuilder::new(document_ids.as_ref(), document_ids.as_ref()).intersection(); - SetOperation::::extend_collection(op, &mut counter); - facets.insert(key, counter.0); - } - result.facets = Some(facets); + if let Some(f) = facet_count_docids { + // hardcoded value, until approximation optimization + result.exhaustive_facet_count = Some(true); + result.facets = Some(facet_count(f, &docids)); } let before = Instant::now(); @@ -216,7 +211,7 @@ pub fn bucket_sort_with_distinct<'c, FI, FD>( query: &str, range: Range, facets_docids: Option>, - facet_count_docids: Option>>>, + facet_count_docids: Option>>>>, filter: Option, distinct: FD, distinct_size: usize, @@ -276,15 +271,10 @@ where docids = Cow::Owned(intersection); } - if let Some(facet_count_docids) = facet_count_docids { - let mut facets = HashMap::new(); - for (key, document_ids) in facet_count_docids { - let mut counter = Counter::new(); - let op = OpBuilder::new(document_ids.as_ref(), document_ids.as_ref()).intersection(); - SetOperation::::extend_collection(op, &mut counter); - facets.insert(key, counter.0); - } - result.facets = Some(facets); + if let Some(f) = facet_count_docids { + // hardcoded value, until approximation optimization + result.exhaustive_facet_count = Some(true); + result.facets = Some(facet_count(f, &docids)); } let before = Instant::now(); @@ -618,3 +608,22 @@ impl Deref for PostingsListView<'_> { } } } + +/// For each entry in facet_docids, calculates the number of documents in the intersection with candidate_docids. +fn facet_count( + facet_docids: HashMap>>>, + candidate_docids: &Set, +) -> HashMap> { + let mut facets_counts = HashMap::with_capacity(facet_docids.len()); + for (key, doc_map) in facet_docids { + let mut count_map = HashMap::with_capacity(doc_map.len()); + for (value, docids) in doc_map { + let mut counter = Counter::new(); + let op = OpBuilder::new(docids.as_ref(), candidate_docids).intersection(); + SetOperation::::extend_collection(op, &mut counter); + count_map.insert(value, counter.0); + } + facets_counts.insert(key, count_map); + } + facets_counts +} diff --git a/meilisearch-core/src/query_builder.rs b/meilisearch-core/src/query_builder.rs index 77c0421d6..5a995030e 100644 --- a/meilisearch-core/src/query_builder.rs +++ b/meilisearch-core/src/query_builder.rs @@ -12,7 +12,7 @@ use crate::facets::FacetFilter; use either::Either; use sdset::SetOperation; -use meilisearch_schema::FieldId; +use meilisearch_schema::{Schema, FieldId}; pub struct QueryBuilder<'c, 'f, 'd, 'i, 'q> { criteria: Criteria<'c>, @@ -21,8 +21,8 @@ pub struct QueryBuilder<'c, 'f, 'd, 'i, 'q> { distinct: Option<(Box Option + 'd>, usize)>, timeout: Option, index: &'i store::Index, - facet_fitlers: Option<&'q FacetFilter>, - facets: Option<&'q [FieldId]>, + facet_filter: Option, + facets: Option>, } impl<'c, 'f, 'd, 'i, 'q> QueryBuilder<'c, 'f, 'd, 'i, 'q> { @@ -34,8 +34,8 @@ impl<'c, 'f, 'd, 'i, 'q> QueryBuilder<'c, 'f, 'd, 'i, 'q> { } /// sets facet attributes to filter on - pub fn set_facet_filters(&mut self, facets: Option<&'q FacetFilter>) { - self.facet_fitlers = facets; + pub fn set_facet_filter(&mut self, facets: Option) { + self.facet_filter = facets; } /// sets facet attributes for which to return the count @@ -54,7 +54,7 @@ impl<'c, 'f, 'd, 'i, 'q> QueryBuilder<'c, 'f, 'd, 'i, 'q> { distinct: None, timeout: None, index, - facet_fitlers: None, + facet_filter: None, facets: None, } } @@ -87,8 +87,9 @@ impl<'c, 'f, 'd, 'i, 'q> QueryBuilder<'c, 'f, 'd, 'i, 'q> { reader: &heed::RoTxn, query: &str, range: Range, + schema: &Schema, ) -> MResult { - let facets_docids = match self.facet_fitlers { + let facets_docids = match self.facet_filter { Some(facets) => { let mut ands = Vec::with_capacity(facets.len()); let mut ors = Vec::new(); @@ -120,14 +121,21 @@ impl<'c, 'f, 'd, 'i, 'q> QueryBuilder<'c, 'f, 'd, 'i, 'q> { None => None }; + // for each field to retrieve the count for, create an HashMap associating the attribute + // value to a set of matching documents. The HashMaps are them collected in another + // HashMap, associating each HashMap to it's field. let facet_count_docids = match self.facets { Some(field_ids) => { let mut facet_count_map = HashMap::new(); for field_id in field_ids { - for pair in self.index.facets.field_document_ids(reader, *field_id)? { - let (facet_key, document_ids) = pair?; - let facet_key_string = facet_key.to_parts(schema)?; - facet_count_map.insert(facet_key, document_ids); + if let Some(field_name) = schema.name(*field_id) { + let mut key_map = HashMap::new(); + for pair in self.index.facets.field_document_ids(reader, *field_id)? { + let (facet_key, document_ids) = pair?; + let value = facet_key.value(); + key_map.insert(value.to_string(), document_ids); + } + facet_count_map.insert(field_name.to_string(), key_map); } } Some(facet_count_map) diff --git a/meilisearch-core/src/store/facets.rs b/meilisearch-core/src/store/facets.rs index 907ceae9b..216b423c9 100644 --- a/meilisearch-core/src/store/facets.rs +++ b/meilisearch-core/src/store/facets.rs @@ -24,7 +24,7 @@ impl Facets { } pub fn field_document_ids<'txn>(&self, reader: &'txn RoTxn, field_id: FieldId) -> ZResult>> { - self.facets.prefix_iter(reader, &FacetKey::new(field_id, "".to_string())) + self.facets.prefix_iter(reader, &FacetKey::new(field_id, String::new())) } pub fn facet_document_ids<'txn>(&self, reader: &'txn RoTxn, facet_key: &FacetKey) -> ZResult>>> { diff --git a/meilisearch-core/src/store/mod.rs b/meilisearch-core/src/store/mod.rs index ddf6614f5..fb3d68849 100644 --- a/meilisearch-core/src/store/mod.rs +++ b/meilisearch-core/src/store/mod.rs @@ -363,7 +363,7 @@ impl Index { QueryBuilder::new(self) } - pub fn query_builder_with_criteria<'c, 'f, 'd, 'fa, 'i, 'q>( + pub fn query_builder_with_criteria<'c, 'f, 'd, 'i>( &'i self, criteria: Criteria<'c>, ) -> QueryBuilder<'c, 'f, 'd, 'i, 'q> { diff --git a/meilisearch-http/Cargo.toml b/meilisearch-http/Cargo.toml index 8e4876397..6b4c95514 100644 --- a/meilisearch-http/Cargo.toml +++ b/meilisearch-http/Cargo.toml @@ -14,7 +14,7 @@ name = "meilisearch" path = "src/main.rs" [features] -default = ["sentry"] +#default = ["sentry"] [dependencies] actix-cors = "0.2.0" diff --git a/meilisearch-http/src/helpers/meilisearch.rs b/meilisearch-http/src/helpers/meilisearch.rs index 13fdea2d4..66ff58b61 100644 --- a/meilisearch-http/src/helpers/meilisearch.rs +++ b/meilisearch-http/src/helpers/meilisearch.rs @@ -157,7 +157,7 @@ impl<'a> SearchBuilder<'a> { query_builder.set_facets(self.facets.as_deref()); let start = Instant::now(); - let result = query_builder.query(reader, &self.query, self.offset..(self.offset + self.limit)); + let result = query_builder.query(reader, &self.query, self.offset..(self.offset + self.limit), &schema); let search_result = result.map_err(ResponseError::search_documents)?; let time_ms = start.elapsed().as_millis() as usize; @@ -247,7 +247,7 @@ impl<'a> SearchBuilder<'a> { exhaustive_nb_hits: search_result.is_exhaustive, processing_time_ms: time_ms, query: self.query.to_string(), - facets: search_result.facets + facets: search_result.facets, }; Ok(results) @@ -332,6 +332,7 @@ pub struct SearchResult { pub exhaustive_nb_hits: bool, pub processing_time_ms: usize, pub query: String, + pub facets: Option>>, } /// returns the start index and the length on the crop. diff --git a/meilisearch-http/src/routes/search.rs b/meilisearch-http/src/routes/search.rs index 0468784f5..731a6ec90 100644 --- a/meilisearch-http/src/routes/search.rs +++ b/meilisearch-http/src/routes/search.rs @@ -33,7 +33,7 @@ struct SearchQuery { filters: Option, matches: Option, facet_filters: Option, - facets: Option + facets: Option, } #[get("/indexes/{index_uid}/search", wrap = "Authentication::Public")] @@ -94,9 +94,12 @@ async fn search_with_url_query( } } - if let Some(ref facets) = params.facets { + if let Some(facets) = ¶ms.facets { match index.main.attributes_for_faceting(&reader)? { - Some(ref attrs) => { search_builder.add_facets(prepare_facet_list(facets, &schema, attrs)?); }, + Some(ref attrs) => { + let field_ids = prepare_facet_list(&facets, &schema, attrs)?; + search_builder.add_facets(field_ids); + }, None => return Err(ResponseError::FacetExpression("can't return facets count, as no facet is set".to_string())) } @@ -162,26 +165,35 @@ async fn search_with_url_query( Ok(HttpResponse::Ok().json(search_builder.search(&reader)?)) } -fn prepare_facet_list<'fa>(facets: &str, schema: &Schema, facet_attrs: &'fa [FieldId]) -> Result, ResponseError> { - let facet_array = serde_json::from_str(facets).expect("do error handling"); // TODO - match facet_array { - Value::Array(facet_array) => { - let wild_card = Value::String("*".to_string()); - if facet_array.iter().any(|it| it == &wild_card) { - return Ok(Vec::from(facet_attrs)); // TODO can make cow? +/// Parses the incoming string into an array of attributes for which to return a count. It returns +/// a Vec of attribute names ascociated with their id. +/// +/// An error is returned if the array is malformed, or if it contains attributes that are +/// unexisting, or not set as facets. +fn prepare_facet_list(facets: &str, schema: &Schema, facet_attrs: &[FieldId]) -> Result, FacetCountError> { + let json_array = serde_json::from_str(facets)?; + match json_array { + Value::Array(vals) => { + let wildcard = Value::String("*".to_string()); + if vals.iter().any(|f| f == &wildcard) { + return Ok(Vec::from(facet_attrs)); } - let mut fields = Vec::with_capacity(facet_attrs.len()); - for v in facet_array { - match v { - Value::String(name) => { - let id = schema.id(&name).expect("not found error"); // TODO - fields.push(id); + let mut field_ids = Vec::new(); + for facet in vals { + match facet { + Value::String(facet) => { + if let Some(id) = schema.id(&facet) { + if !facet_attrs.contains(&id) { + return Err(ResponseError::FacetExpression("Only attributes set as facet can be counted".to_string())); // TODO make special error + } + field_ids.push(id); + } } - _ => todo!("expected string, found {}", v), + bad_val => return Err(ResponseError::FacetExpression(format!("expected String found {}", bad_val))) } } - return Ok(fields); + Ok(field_ids) } - _ => todo!("error, bad syntax, expected array") + bad_val => return Err(ResponseError::FacetExpression(format!("expected Array found {}", bad_val))) } } From 347045adf28d4fe4b276af45154a5cb60d04e34a Mon Sep 17 00:00:00 2001 From: mpostma Date: Tue, 12 May 2020 12:19:44 +0200 Subject: [PATCH 3/8] smarter field_id name passing --- meilisearch-core/src/query_builder.rs | 25 +++++++++------------ meilisearch-core/src/store/mod.rs | 2 +- meilisearch-http/src/helpers/meilisearch.rs | 17 +++++++------- meilisearch-http/src/routes/search.rs | 10 ++++++--- 4 files changed, 28 insertions(+), 26 deletions(-) diff --git a/meilisearch-core/src/query_builder.rs b/meilisearch-core/src/query_builder.rs index 5a995030e..c323b8c1f 100644 --- a/meilisearch-core/src/query_builder.rs +++ b/meilisearch-core/src/query_builder.rs @@ -12,9 +12,9 @@ use crate::facets::FacetFilter; use either::Either; use sdset::SetOperation; -use meilisearch_schema::{Schema, FieldId}; +use meilisearch_schema::FieldId; -pub struct QueryBuilder<'c, 'f, 'd, 'i, 'q> { +pub struct QueryBuilder<'c, 'f, 'd, 'i> { criteria: Criteria<'c>, searchable_attrs: Option, filter: Option bool + 'f>>, @@ -25,7 +25,7 @@ pub struct QueryBuilder<'c, 'f, 'd, 'i, 'q> { facets: Option>, } -impl<'c, 'f, 'd, 'i, 'q> QueryBuilder<'c, 'f, 'd, 'i, 'q> { +impl<'c, 'f, 'd, 'i> QueryBuilder<'c, 'f, 'd, 'i> { pub fn new(index: &'i store::Index) -> Self { QueryBuilder::with_criteria( index, @@ -39,7 +39,7 @@ impl<'c, 'f, 'd, 'i, 'q> QueryBuilder<'c, 'f, 'd, 'i, 'q> { } /// sets facet attributes for which to return the count - pub fn set_facets(&mut self, facets: Option<&'q [FieldId]>) { + pub fn set_facets(&mut self, facets: Option>) { self.facets = facets; } @@ -87,7 +87,6 @@ impl<'c, 'f, 'd, 'i, 'q> QueryBuilder<'c, 'f, 'd, 'i, 'q> { reader: &heed::RoTxn, query: &str, range: Range, - schema: &Schema, ) -> MResult { let facets_docids = match self.facet_filter { Some(facets) => { @@ -127,16 +126,14 @@ impl<'c, 'f, 'd, 'i, 'q> QueryBuilder<'c, 'f, 'd, 'i, 'q> { let facet_count_docids = match self.facets { Some(field_ids) => { let mut facet_count_map = HashMap::new(); - for field_id in field_ids { - if let Some(field_name) = schema.name(*field_id) { - let mut key_map = HashMap::new(); - for pair in self.index.facets.field_document_ids(reader, *field_id)? { - let (facet_key, document_ids) = pair?; - let value = facet_key.value(); - key_map.insert(value.to_string(), document_ids); - } - facet_count_map.insert(field_name.to_string(), key_map); + for (field_id, field_name) in field_ids { + let mut key_map = HashMap::new(); + for pair in self.index.facets.field_document_ids(reader, field_id)? { + let (facet_key, document_ids) = pair?; + let value = facet_key.value(); + key_map.insert(value.to_string(), document_ids); } + facet_count_map.insert(field_name, key_map); } Some(facet_count_map) } diff --git a/meilisearch-core/src/store/mod.rs b/meilisearch-core/src/store/mod.rs index fb3d68849..6448a3441 100644 --- a/meilisearch-core/src/store/mod.rs +++ b/meilisearch-core/src/store/mod.rs @@ -366,7 +366,7 @@ impl Index { pub fn query_builder_with_criteria<'c, 'f, 'd, 'i>( &'i self, criteria: Criteria<'c>, - ) -> QueryBuilder<'c, 'f, 'd, 'i, 'q> { + ) -> QueryBuilder<'c, 'f, 'd, 'i> { QueryBuilder::with_criteria(self, criteria) } } diff --git a/meilisearch-http/src/helpers/meilisearch.rs b/meilisearch-http/src/helpers/meilisearch.rs index 66ff58b61..e50fbcaad 100644 --- a/meilisearch-http/src/helpers/meilisearch.rs +++ b/meilisearch-http/src/helpers/meilisearch.rs @@ -52,7 +52,7 @@ pub struct SearchBuilder<'a> { filters: Option, matches: bool, facet_filters: Option, - facets: Option> + facets: Option> } impl<'a> SearchBuilder<'a> { @@ -102,12 +102,12 @@ impl<'a> SearchBuilder<'a> { self } - pub fn add_facets(&mut self, facets: Vec) -> &SearchBuilder { + pub fn add_facets(&mut self, facets: Vec<(FieldId, String)>) -> &SearchBuilder { self.facets = Some(facets); self } - pub fn search(&self, reader: &heed::RoTxn) -> Result { + pub fn search(self, reader: &heed::RoTxn) -> Result { let schema = self .index .main @@ -124,8 +124,8 @@ impl<'a> SearchBuilder<'a> { if let Some(filter_expression) = &self.filters { let filter = Filter::parse(filter_expression, &schema)?; + let index = &self.index; query_builder.with_filter(move |id| { - let index = &self.index; let reader = &reader; let filter = &filter; match filter.test(reader, index, id) { @@ -140,8 +140,9 @@ impl<'a> SearchBuilder<'a> { if let Some(field) = self.index.main.distinct_attribute(reader)? { if let Some(field_id) = schema.id(&field) { + let index = &self.index; query_builder.with_distinct(1, move |id| { - match self.index.document_attribute_bytes(reader, id, field_id) { + match index.document_attribute_bytes(reader, id, field_id) { Ok(Some(bytes)) => { let mut s = SipHasher::new(); bytes.hash(&mut s); @@ -153,11 +154,11 @@ impl<'a> SearchBuilder<'a> { } } - query_builder.set_facet_filters(self.facet_filters.as_ref()); - query_builder.set_facets(self.facets.as_deref()); + query_builder.set_facet_filters(self.facet_filters); + query_builder.set_facets(self.facets); let start = Instant::now(); - let result = query_builder.query(reader, &self.query, self.offset..(self.offset + self.limit), &schema); + let result = query_builder.query(reader, &self.query, self.offset..(self.offset + self.limit)); let search_result = result.map_err(ResponseError::search_documents)?; let time_ms = start.elapsed().as_millis() as usize; diff --git a/meilisearch-http/src/routes/search.rs b/meilisearch-http/src/routes/search.rs index 731a6ec90..5d086d128 100644 --- a/meilisearch-http/src/routes/search.rs +++ b/meilisearch-http/src/routes/search.rs @@ -176,9 +176,13 @@ fn prepare_facet_list(facets: &str, schema: &Schema, facet_attrs: &[FieldId]) -> Value::Array(vals) => { let wildcard = Value::String("*".to_string()); if vals.iter().any(|f| f == &wildcard) { - return Ok(Vec::from(facet_attrs)); + let attrs = facet_attrs + .iter() + .filter_map(|&id| schema.name(id).map(|n| (id, n.to_string()))) + .collect(); + return Ok(attrs); } - let mut field_ids = Vec::new(); + let mut field_ids = Vec::with_capacity(facet_attrs.len()); for facet in vals { match facet { Value::String(facet) => { @@ -186,7 +190,7 @@ fn prepare_facet_list(facets: &str, schema: &Schema, facet_attrs: &[FieldId]) -> if !facet_attrs.contains(&id) { return Err(ResponseError::FacetExpression("Only attributes set as facet can be counted".to_string())); // TODO make special error } - field_ids.push(id); + field_ids.push((id, facet)); } } bad_val => return Err(ResponseError::FacetExpression(format!("expected String found {}", bad_val))) From 869b6019c6e87b68588b046d9044f51e6672c3ea Mon Sep 17 00:00:00 2001 From: mpostma Date: Tue, 12 May 2020 12:37:16 +0200 Subject: [PATCH 4/8] fix tests --- meilisearch-core/examples/from_file.rs | 6 +- meilisearch-core/src/database.rs | 9 +- meilisearch-core/src/query_builder.rs | 125 ++++++++++---------- meilisearch-http/src/helpers/meilisearch.rs | 2 +- 4 files changed, 72 insertions(+), 70 deletions(-) diff --git a/meilisearch-core/examples/from_file.rs b/meilisearch-core/examples/from_file.rs index ed3711960..ad319442e 100644 --- a/meilisearch-core/examples/from_file.rs +++ b/meilisearch-core/examples/from_file.rs @@ -371,12 +371,12 @@ fn search_command(command: SearchCommand, database: Database) -> Result<(), Box< }); } - let (documents, _nb_hits) = builder.query(ref_reader, &query, 0..command.number_results)?; + let result = builder.query(ref_reader, &query, 0..command.number_results)?; let mut retrieve_duration = Duration::default(); - let number_of_documents = documents.len(); - for mut doc in documents { + let number_of_documents = result.documents.len(); + for mut doc in result.documents { doc.highlights .sort_unstable_by_key(|m| (m.char_index, m.char_length)); diff --git a/meilisearch-core/src/database.rs b/meilisearch-core/src/database.rs index 3e79bc04d..d7f78dca1 100644 --- a/meilisearch-core/src/database.rs +++ b/meilisearch-core/src/database.rs @@ -371,6 +371,7 @@ impl Database { mod tests { use super::*; + use crate::bucket_sort::SortResult; use crate::criterion::{self, CriteriaBuilder}; use crate::update::{ProcessedUpdateResult, UpdateStatus}; use crate::settings::Settings; @@ -675,8 +676,8 @@ mod tests { // even try to search for a document let reader = db.main_read_txn().unwrap(); - let (results, _nb_hits) = index.query_builder().query(&reader, "21 ", 0..20).unwrap(); - assert_matches!(results.len(), 1); + let SortResult {documents, .. } = index.query_builder().query(&reader, "21 ", 0..20).unwrap(); + assert_matches!(documents.len(), 1); reader.abort(); @@ -1073,8 +1074,8 @@ mod tests { let builder = index.query_builder_with_criteria(criteria); - let (results, _nb_hits) = builder.query(&reader, "Kevin", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "Kevin", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!( iter.next(), diff --git a/meilisearch-core/src/query_builder.rs b/meilisearch-core/src/query_builder.rs index c323b8c1f..69a1ceff6 100644 --- a/meilisearch-core/src/query_builder.rs +++ b/meilisearch-core/src/query_builder.rs @@ -193,6 +193,7 @@ mod tests { use tempfile::TempDir; use crate::DocIndex; + use crate::Document; use crate::automaton::normalize_str; use crate::bucket_sort::SimpleMatch; use crate::database::{Database,DatabaseOptions}; @@ -383,8 +384,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "iphone from apple", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult { documents, .. } = builder.query(&reader, "iphone from apple", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut matches = matches.into_iter(); @@ -406,8 +407,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "hello", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult { documents, .. } = builder.query(&reader, "hello", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut matches = matches.into_iter(); @@ -417,8 +418,8 @@ mod tests { assert_matches!(iter.next(), None); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "bonjour", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult { documents, .. } = builder.query(&reader, "bonjour", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut matches = matches.into_iter(); @@ -440,7 +441,7 @@ mod tests { // let builder = store.query_builder(); // let results = builder.query(&reader, "sal", 0..20).unwrap(); - // let mut iter = results.into_iter(); + // let mut iter = documents.into_iter(); // assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { // let mut matches = matches.into_iter(); @@ -451,7 +452,7 @@ mod tests { // let builder = store.query_builder(); // let results = builder.query(&reader, "bonj", 0..20).unwrap(); - // let mut iter = results.into_iter(); + // let mut iter = documents.into_iter(); // assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { // let mut matches = matches.into_iter(); @@ -462,13 +463,13 @@ mod tests { // let builder = store.query_builder(); // let results = builder.query(&reader, "sal blabla", 0..20).unwrap(); - // let mut iter = results.into_iter(); + // let mut iter = documents.into_iter(); // assert_matches!(iter.next(), None); // let builder = store.query_builder(); // let results = builder.query(&reader, "bonj blabla", 0..20).unwrap(); - // let mut iter = results.into_iter(); + // let mut iter = documents.into_iter(); // assert_matches!(iter.next(), None); // } @@ -484,7 +485,7 @@ mod tests { // let builder = store.query_builder(); // let results = builder.query(&reader, "salutution", 0..20).unwrap(); - // let mut iter = results.into_iter(); + // let mut iter = documents.into_iter(); // assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { // let mut matches = matches.into_iter(); @@ -495,7 +496,7 @@ mod tests { // let builder = store.query_builder(); // let results = builder.query(&reader, "saluttion", 0..20).unwrap(); - // let mut iter = results.into_iter(); + // let mut iter = documents.into_iter(); // assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { // let mut matches = matches.into_iter(); @@ -521,8 +522,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "hello", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult { documents, .. } = builder.query(&reader, "hello", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut matches = matches.into_iter(); @@ -542,8 +543,8 @@ mod tests { assert_matches!(iter.next(), None); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "bonjour", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult { documents, .. } = builder.query(&reader, "bonjour", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut matches = matches.into_iter(); @@ -563,8 +564,8 @@ mod tests { assert_matches!(iter.next(), None); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "salut", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult { documents, .. } = builder.query(&reader, "salut", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut matches = matches.into_iter(); @@ -609,8 +610,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "NY subway", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult { documents, .. } = builder.query(&reader, "NY subway", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(1), matches, .. }) => { let mut iter = matches.into_iter(); @@ -631,8 +632,8 @@ mod tests { assert_matches!(iter.next(), None); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "NYC subway", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult { documents, .. } = builder.query(&reader, "NYC subway", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(1), matches, .. }) => { let mut iter = matches.into_iter(); @@ -673,8 +674,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "NY", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "NY", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(2), matches, .. }) => { let mut matches = matches.into_iter(); @@ -697,8 +698,8 @@ mod tests { assert_matches!(iter.next(), None); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "new york", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "new york", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut matches = matches.into_iter(); @@ -731,8 +732,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "NY subway", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "NY subway", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut matches = matches.into_iter(); @@ -748,8 +749,8 @@ mod tests { assert_matches!(iter.next(), None); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "new york subway", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "new york subway", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(1), matches, .. }) => { let mut matches = matches.into_iter(); @@ -796,8 +797,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "NY subway", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "NY subway", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(1), matches, .. }) => { let mut iter = matches.into_iter(); @@ -818,8 +819,8 @@ mod tests { assert_matches!(iter.next(), None); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "NYC subway", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "NYC subway", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(1), matches, .. }) => { let mut iter = matches.into_iter(); @@ -871,8 +872,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "NY subway broken", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "NY subway broken", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut iter = matches.into_iter(); @@ -887,8 +888,8 @@ mod tests { assert_matches!(iter.next(), None); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "NYC subway", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "NYC subway", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(1), matches, .. }) => { let mut iter = matches.into_iter(); @@ -943,10 +944,10 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder + let SortResult {documents, .. } = builder .query(&reader, "new york underground train broken", 0..20) .unwrap(); - let mut iter = results.into_iter(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(2), matches, .. }) => { let mut matches = matches.into_iter(); @@ -973,10 +974,10 @@ mod tests { assert_matches!(iter.next(), None); let builder = store.query_builder(); - let (results, _nb_hits) = builder + let SortResult {documents, .. } = builder .query(&reader, "new york city underground train broken", 0..20) .unwrap(); - let mut iter = results.into_iter(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(2), matches, .. }) => { let mut matches = matches.into_iter(); @@ -1017,8 +1018,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "new york big ", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "new york big ", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut matches = matches.into_iter(); @@ -1051,8 +1052,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "NY subway ", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "NY subway ", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut matches = matches.into_iter(); @@ -1101,10 +1102,10 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder + let SortResult {documents, .. } = builder .query(&reader, "new york city long subway cool ", 0..20) .unwrap(); - let mut iter = results.into_iter(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut matches = matches.into_iter(); @@ -1134,8 +1135,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "telephone", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "telephone", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut iter = matches.into_iter(); @@ -1151,8 +1152,8 @@ mod tests { assert_matches!(iter.next(), None); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "téléphone", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "téléphone", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut iter = matches.into_iter(); @@ -1168,8 +1169,8 @@ mod tests { assert_matches!(iter.next(), None); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "télephone", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "télephone", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(1), matches, .. }) => { let mut iter = matches.into_iter(); @@ -1195,8 +1196,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "i phone case", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "i phone case", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut iter = matches.into_iter(); @@ -1224,8 +1225,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "searchengine", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "searchengine", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut iter = matches.into_iter(); @@ -1264,8 +1265,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "searchengine", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "searchengine", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut iter = matches.into_iter(); @@ -1296,8 +1297,8 @@ mod tests { let reader = db.main_read_txn().unwrap(); let builder = store.query_builder(); - let (results, _nb_hits) = builder.query(&reader, "searchengine", 0..20).unwrap(); - let mut iter = results.into_iter(); + let SortResult {documents, .. } = builder.query(&reader, "searchengine", 0..20).unwrap(); + let mut iter = documents.into_iter(); assert_matches!(iter.next(), Some(Document { id: DocumentId(0), matches, .. }) => { let mut iter = matches.into_iter(); diff --git a/meilisearch-http/src/helpers/meilisearch.rs b/meilisearch-http/src/helpers/meilisearch.rs index e50fbcaad..d944adf3b 100644 --- a/meilisearch-http/src/helpers/meilisearch.rs +++ b/meilisearch-http/src/helpers/meilisearch.rs @@ -154,7 +154,7 @@ impl<'a> SearchBuilder<'a> { } } - query_builder.set_facet_filters(self.facet_filters); + query_builder.set_facet_filter(self.facet_filters); query_builder.set_facets(self.facets); let start = Instant::now(); From 5051a796a082877dd84e445ab769951af3245a83 Mon Sep 17 00:00:00 2001 From: mpostma Date: Tue, 12 May 2020 14:26:43 +0200 Subject: [PATCH 5/8] error handling --- meilisearch-http/src/error.rs | 59 +++++++++++++++++++++++---- meilisearch-http/src/routes/search.rs | 11 +++-- 2 files changed, 55 insertions(+), 15 deletions(-) diff --git a/meilisearch-http/src/error.rs b/meilisearch-http/src/error.rs index e79cb56b6..eee9d8e8e 100644 --- a/meilisearch-http/src/error.rs +++ b/meilisearch-http/src/error.rs @@ -27,6 +27,40 @@ pub enum ResponseError { PayloadTooLarge, UnsupportedMediaType, FacetExpression(String), + FacetCount(String), +} + +pub enum FacetCountError { + AttributeNotSet(String), + SyntaxError(String), + UnexpectedToken { found: String, expected: &'static [&'static str] }, + NoFacetSet, +} + +impl FacetCountError { + pub fn unexpected_token(found: impl ToString, expected: &'static [&'static str]) -> FacetCountError { + let found = found.to_string(); + FacetCountError::UnexpectedToken { expected, found } + } +} + +impl From for FacetCountError { + fn from(other: serde_json::error::Error) -> FacetCountError { + FacetCountError::SyntaxError(other.to_string()) + } +} + +impl fmt::Display for FacetCountError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use FacetCountError::*; + + match self { + AttributeNotSet(attr) => write!(f, "attribute {} is not set as facet", attr), + SyntaxError(msg) => write!(f, "syntax error: {}", msg), + UnexpectedToken { expected, found } => write!(f, "unexpected {} found, expected {:?}", found, expected), + NoFacetSet => write!(f, "can't perform facet count, as no facet is set"), + } + } } impl ResponseError { @@ -113,6 +147,7 @@ impl fmt::Display for ResponseError { Self::FacetExpression(e) => write!(f, "error parsing facet filter expression: {}", e), Self::PayloadTooLarge => f.write_str("Payload to large"), Self::UnsupportedMediaType => f.write_str("Unsupported media type") + Self::FacetCount(e) => write!(f, "error with facet count: {}", e), } } } @@ -134,6 +169,7 @@ impl aweb::error::ResponseError for ResponseError { | Self::RetrieveDocument(_, _) | Self::FacetExpression(_) | Self::SearchDocuments(_) + | Self::FacetCount(_) | Self::FilterParsing(_) => StatusCode::BAD_REQUEST, Self::DocumentNotFound(_) | Self::IndexNotFound(_) @@ -198,18 +234,23 @@ impl From for ResponseError { } } -impl From for ResponseError { - fn from(err: JsonPayloadError) -> ResponseError { - match err { - JsonPayloadError::Deserialize(err) => ResponseError::BadRequest(format!("Invalid JSON: {}", err)), - JsonPayloadError::Overflow => ResponseError::PayloadTooLarge, - JsonPayloadError::ContentType => ResponseError::UnsupportedMediaType, - JsonPayloadError::Payload(err) => ResponseError::BadRequest(format!("Problem while decoding the request: {}", err)), - } +impl From for ResponseError { + fn from(other: FacetCountError) -> ResponseError { + ResponseError::FacetCount(other.to_string()) } } +impl From for ResponseError { + fn from(err: JsonPayloadError) -> ResponseError { + match err { + JsonPayloadError::Deserialize(err) => ResponseError::BadRequest(format!("Invalid JSON: {}", err)), + JsonPayloadError::Overflow => ResponseError::PayloadTooLarge, + JsonPayloadError::ContentType => ResponseError::UnsupportedMediaType, + JsonPayloadError::Payload(err) => ResponseError::BadRequest(format!("Problem while decoding the request: {}", err)), + } + } +} pub fn json_error_handler(err: JsonPayloadError) -> ResponseError { - err.into() + err.into() } diff --git a/meilisearch-http/src/routes/search.rs b/meilisearch-http/src/routes/search.rs index 5d086d128..7739ea1d8 100644 --- a/meilisearch-http/src/routes/search.rs +++ b/meilisearch-http/src/routes/search.rs @@ -7,7 +7,7 @@ use actix_web_macros::get; use serde::Deserialize; use serde_json::Value; -use crate::error::ResponseError; +use crate::error::{ResponseError, FacetCountError}; use crate::helpers::meilisearch::IndexSearchExt; use crate::helpers::Authentication; use crate::routes::IndexParam; @@ -100,9 +100,8 @@ async fn search_with_url_query( let field_ids = prepare_facet_list(&facets, &schema, attrs)?; search_builder.add_facets(field_ids); }, - None => return Err(ResponseError::FacetExpression("can't return facets count, as no facet is set".to_string())) + None => return Err(FacetCountError::NoFacetSet.into()) } - } if let Some(attributes_to_crop) = ¶ms.attributes_to_crop { @@ -188,16 +187,16 @@ fn prepare_facet_list(facets: &str, schema: &Schema, facet_attrs: &[FieldId]) -> Value::String(facet) => { if let Some(id) = schema.id(&facet) { if !facet_attrs.contains(&id) { - return Err(ResponseError::FacetExpression("Only attributes set as facet can be counted".to_string())); // TODO make special error + return Err(FacetCountError::AttributeNotSet(facet)); } field_ids.push((id, facet)); } } - bad_val => return Err(ResponseError::FacetExpression(format!("expected String found {}", bad_val))) + bad_val => return Err(FacetCountError::unexpected_token(bad_val, &["String"])), } } Ok(field_ids) } - bad_val => return Err(ResponseError::FacetExpression(format!("expected Array found {}", bad_val))) + bad_val => return Err(FacetCountError::unexpected_token(bad_val, &["[String]"])) } } From f38d0d731ff86afcc9b4f15cdc4bc2a665bba6b3 Mon Sep 17 00:00:00 2001 From: mpostma Date: Tue, 12 May 2020 14:36:28 +0200 Subject: [PATCH 6/8] style fix --- meilisearch-core/src/bucket_sort.rs | 2 +- meilisearch-core/src/query_builder.rs | 11 +++++------ meilisearch-http/Cargo.toml | 2 +- meilisearch-http/src/helpers/meilisearch.rs | 2 +- 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/meilisearch-core/src/bucket_sort.rs b/meilisearch-core/src/bucket_sort.rs index 337bd8cce..4fd6b3080 100644 --- a/meilisearch-core/src/bucket_sort.rs +++ b/meilisearch-core/src/bucket_sort.rs @@ -28,7 +28,7 @@ use crate::query_tree::Context as QTContext; pub struct SortResult { pub documents: Vec, pub nb_hits: usize, - pub is_exhaustive: bool, + pub exhaustive_nb_hit: bool, pub facets: Option>>, pub exhaustive_facet_count: Option, } diff --git a/meilisearch-core/src/query_builder.rs b/meilisearch-core/src/query_builder.rs index 69a1ceff6..1f1ef28e3 100644 --- a/meilisearch-core/src/query_builder.rs +++ b/meilisearch-core/src/query_builder.rs @@ -3,17 +3,17 @@ use std::collections::HashMap; use std::ops::{Range, Deref}; use std::time::Duration; +use either::Either; +use sdset::SetOperation; + +use meilisearch_schema::FieldId; + use crate::database::MainT; use crate::bucket_sort::{bucket_sort, bucket_sort_with_distinct, SortResult}; use crate::{criterion::Criteria, DocumentId}; use crate::{reordered_attrs::ReorderedAttrs, store, MResult}; use crate::facets::FacetFilter; -use either::Either; -use sdset::SetOperation; - -use meilisearch_schema::FieldId; - pub struct QueryBuilder<'c, 'f, 'd, 'i> { criteria: Criteria<'c>, searchable_attrs: Option, @@ -140,7 +140,6 @@ impl<'c, 'f, 'd, 'i> QueryBuilder<'c, 'f, 'd, 'i> { None => None, }; - match self.distinct { Some((distinct, distinct_size)) => bucket_sort_with_distinct( reader, diff --git a/meilisearch-http/Cargo.toml b/meilisearch-http/Cargo.toml index 6b4c95514..8e4876397 100644 --- a/meilisearch-http/Cargo.toml +++ b/meilisearch-http/Cargo.toml @@ -14,7 +14,7 @@ name = "meilisearch" path = "src/main.rs" [features] -#default = ["sentry"] +default = ["sentry"] [dependencies] actix-cors = "0.2.0" diff --git a/meilisearch-http/src/helpers/meilisearch.rs b/meilisearch-http/src/helpers/meilisearch.rs index d944adf3b..10fdd9c21 100644 --- a/meilisearch-http/src/helpers/meilisearch.rs +++ b/meilisearch-http/src/helpers/meilisearch.rs @@ -245,7 +245,7 @@ impl<'a> SearchBuilder<'a> { offset: self.offset, limit: self.limit, nb_hits: search_result.nb_hits, - exhaustive_nb_hits: search_result.is_exhaustive, + exhaustive_nb_hits: search_result.exhaustive_nb_hit, processing_time_ms: time_ms, query: self.query.to_string(), facets: search_result.facets, From 28a3e4005a6cefe7745ce759d9157e9ff3a25458 Mon Sep 17 00:00:00 2001 From: mpostma Date: Tue, 12 May 2020 15:23:35 +0200 Subject: [PATCH 7/8] adds test --- meilisearch-http/src/error.rs | 2 +- meilisearch-http/tests/search.rs | 51 ++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/meilisearch-http/src/error.rs b/meilisearch-http/src/error.rs index eee9d8e8e..295931454 100644 --- a/meilisearch-http/src/error.rs +++ b/meilisearch-http/src/error.rs @@ -146,7 +146,7 @@ impl fmt::Display for ResponseError { Self::SearchDocuments(err) => write!(f, "impossible to search documents; {}", err), Self::FacetExpression(e) => write!(f, "error parsing facet filter expression: {}", e), Self::PayloadTooLarge => f.write_str("Payload to large"), - Self::UnsupportedMediaType => f.write_str("Unsupported media type") + Self::UnsupportedMediaType => f.write_str("Unsupported media type"), Self::FacetCount(e) => write!(f, "error with facet count: {}", e), } } diff --git a/meilisearch-http/tests/search.rs b/meilisearch-http/tests/search.rs index 635f46560..58a66cd39 100644 --- a/meilisearch-http/tests/search.rs +++ b/meilisearch-http/tests/search.rs @@ -1292,3 +1292,54 @@ async fn test_faceted_search_invalid() { let (_response, status_code) = server.search(query).await; assert_ne!(status_code, 202); } + +#[actix_rt::test] +async fn test_facet_count() { + let mut server = common::Server::test_server().await; + + // test no facets set, search on color + let query = "q=a&facets=%5B%22color%22%5D"; + let (_response, status_code) = server.search(query).await; + assert_eq!(status_code, 400); + + let body = json!({ + "attributesForFaceting": ["color", "tags"] + }); + server.update_all_settings(body).await; + // same as before, but now facets are set: + let (response, _status_code) = server.search(query).await; + assert_eq!(response.get("facets").unwrap().as_object().unwrap().values().count(), 1); + // searching on color and tags + let query = "q=a&facets=%5B%22color%22,%20%22tags%22%5D"; + let (response, _status_code) = server.search(query).await; + let facets = response.get("facets").unwrap().as_object().unwrap(); + eprintln!("response: {:#?}", response); + assert_eq!(facets.values().count(), 2); + assert_ne!(!facets.get("color").unwrap().as_object().unwrap().values().count(), 0); + assert_ne!(!facets.get("tags").unwrap().as_object().unwrap().values().count(), 0); + // wildcard + let query = "q=a&facets=%5B%22*%22%5D"; + let (response, _status_code) = server.search(query).await; + assert_eq!(response.get("facets").unwrap().as_object().unwrap().values().count(), 2); + // wildcard with other attributes: + let query = "q=a&facets=%5B%22color%22,%20%22*%22%5D"; + let (response, _status_code) = server.search(query).await; + assert_eq!(response.get("facets").unwrap().as_object().unwrap().values().count(), 2); + // empty facet list + let query = "q=a&facets=%5B%5D"; + let (response, _status_code) = server.search(query).await; + assert_eq!(response.get("facets").unwrap().as_object().unwrap().values().count(), 0); + + // attr not set as facet passed: + let query = "q=a&facets=%5B%22gender%22%5D"; + let (_response, status_code) = server.search(query).await; + assert_eq!(status_code, 400); + // string instead of array: + let query = "q=a&facets=%22gender%22"; + let (_response, status_code) = server.search(query).await; + assert_eq!(status_code, 400); + // invalid value in array: + let query = "q=a&facets=%5B%22color%22,%20true%5D"; + let (_response, status_code) = server.search(query).await; + assert_eq!(status_code, 400); +} From eca39ad7bf75e11b47fba999b080068aaf446545 Mon Sep 17 00:00:00 2001 From: mpostma Date: Tue, 12 May 2020 17:09:52 +0200 Subject: [PATCH 8/8] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0b1ace8d..69be1a4d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## v0.10.2 + - Add support for facet count (#676) - Add support for faceted search (#631) - Add support for configuring the lmdb map size (#646, #647) - Add exposed port for Dockerfile (#654)