Facet filters #631

MarinPostma · 2020-04-23T12:58:36Z

#625

MarinPostma · 2020-05-05T21:14:07Z

will do remaining facet count in another PR ;)

meilisearch-core/src/facets.rs

meilisearch-core/src/error.rs

meilisearch-core/src/facets.rs

meilisearch-http/src/routes/setting.rs

meilisearch-schema/Cargo.toml

meilisearch-schema/src/lib.rs

meilisearch-http/Cargo.toml

meilisearch-core/src/query_builder.rs

meilisearch-core/Cargo.toml

meilisearch-core/src/error.rs

meilisearch-core/src/facets.rs

meilisearch-core/src/store/mod.rs

Kerollmops · 2020-05-07T14:33:01Z

meilisearch-core/src/update/documents_addition.rs

+        let facet_map = facets::facet_map_from_docids(writer, &index, &documents_ids_to_reindex, &attributes_for_facetting)?;
+        index.facets.add(writer, facet_map)?;
+    }
+    // ^-- merge? --v


What this comment means again?

what facet map does could be done inside the loop under the comment, and save us a few db reads. this would be done at the price of less readability, and would make it harder to move this logic out if we wanted it. This comment sole purpose is to tell us that this optimization is possible, is we want it, as it is not trivial.

meilisearch-http/src/routes/search.rs

meilisearch-schema/src/lib.rs

meilisearch-core/src/facets.rs

meilisearch-core/src/store/mod.rs

MarinPostma · 2020-05-11T09:06:47Z

don't merge !

MarinPostma · 2020-05-11T10:42:30Z

regarding document reindexation (meilisearch-core/src/update/documents_addition.rs:250), what facet map does could be done inside the loop under the comment, and save us a few db reads. this would be done at the price of less readability, and would make it harder to move this logic out if we wanted it. This comment sole purpose is to tell us that this optimization is possible, is we want it, as it is not trivial.

Kerollmops · 2020-05-11T11:12:02Z

meilisearch-core/src/facets.rs

-        v => v.to_string(),
+        // ignore null
+        Value::Null => return Ok(()),
+        _ => return Err(FacetError::InvalidDocumentAttribute(document_id)),


Would be great if you show the invalid field and the fact that we only support strings and arrays of strings only.

Kerollmops · 2020-05-11T11:22:02Z

meilisearch-core/src/facets.rs

+                                    bad_value => {
+                                        return Err(FacetError::UnexpectedToken { expected: &["String"], found: bad_value.to_string() })
+                                    }
+                                }
+                            }
+                            filter.push(Either::Left(inner));
+                        }
+                        bad_value => return Err(FacetError::UnexpectedToken { expected: &["String", "Array"], found: bad_value.to_string() }),
+                    }
+                }
+                return Ok(Self(filter));
+            }
+            bad_value => Err(FacetError::UnexpectedToken { expected: &["Array"], found: bad_value.to_string() }),
+        }


Could you create a contructor for the unexpected token, this will reduce the amount of code here.

meilisearch-core/src/facets.rs

Kerollmops · 2020-05-11T13:30:38Z

meilisearch-core/src/error.rs

+    pub fn unexpected_token(expected: &'static [&'static str], found: String) -> FacetError {
+        FacetError::UnexpectedToken{ expected, found }
+    }
+
+    pub fn attribute_not_set(expected: Vec<String>, found: String) -> FacetError {
+        FacetError::AttributeNotSet{ expected, found }
+    }


We can reduce the line length of the other file by avoiding calling to_string and transform it there.

Suggested change

pub fn unexpected_token(expected: &'static [&'static str], found: String) -> FacetError {

FacetError::UnexpectedToken{ expected, found }

}

pub fn attribute_not_set(expected: Vec<String>, found: String) -> FacetError {

FacetError::AttributeNotSet{ expected, found }

}

pub fn unexpected_token(expected: &'static [&'static str], found: impl ToString) -> FacetError {

FacetError::UnexpectedToken{ expected, found: found.to_string() }

}

pub fn attribute_not_set(expected: Vec<String>, found: impl ToString) -> FacetError {

FacetError::AttributeNotSet{ expected, foundL found.to_string() }

}

Kerollmops

I think this is good now, I would like you to rebase and I will merge :)

Kerollmops · 2020-05-11T14:31:54Z

meilisearch-core/src/facets.rs

+                }
+                return Ok(Self(filter));
+            }
+            bad_value => Err(FacetError::unexpected_token(&["String"], bad_value)),


Suggested change

bad_value => Err(FacetError::unexpected_token(&["String"], bad_value)),

bad_value => Err(FacetError::unexpected_token(&["Array"], bad_value)),

Kerollmops · 2020-05-11T14:32:40Z

meilisearch-http/Cargo.toml

@@ -26,6 +26,7 @@ actix-web = "2"
 actix-web-macros = "0.1.0"
 chrono = { version = "0.4.11", features = ["serde"] }
 crossbeam-channel = "0.4.2"
+either = { version = "1.5.3", features = ["serde"] }


Are you using this dependency in meilisearch-http?

631: Revert "Remove Bors required test for Windows" r=Kerollmops a=curquiza Reverts meilisearch/milli#612 Because the issue does not seem to be there! Closes meilisearch/milli#614 Co-authored-by: Clémentine Urquizar - curqui <clementine@meilisearch.com>

MarinPostma changed the base branch from master to tide-to-actix-web April 23, 2020 13:00

qdequele force-pushed the tide-to-actix-web branch 3 times, most recently from 93ba8dd to 5cbadb4 Compare April 26, 2020 18:54

MarinPostma force-pushed the facet-filters branch from c7dc5ae to 22e50fe Compare April 27, 2020 21:53

qdequele force-pushed the tide-to-actix-web branch from 5cbadb4 to 99866ba Compare April 28, 2020 15:55

MarinPostma force-pushed the facet-filters branch from 22e50fe to 4b5f82d Compare April 29, 2020 21:02

MarinPostma changed the base branch from tide-to-actix-web to master April 29, 2020 21:03

MarinPostma force-pushed the facet-filters branch 4 times, most recently from 2fb9d11 to 1ff80c8 Compare May 5, 2020 20:31

MarinPostma marked this pull request as ready for review May 5, 2020 21:13

MarinPostma requested a review from Kerollmops May 5, 2020 21:17

Kerollmops requested changes May 6, 2020

View reviewed changes

Kerollmops requested changes May 7, 2020

View reviewed changes

meilisearch-core/src/facets.rs Outdated Show resolved Hide resolved

meilisearch-core/src/facets.rs Show resolved Hide resolved

meilisearch-core/src/store/mod.rs Show resolved Hide resolved

MarinPostma force-pushed the facet-filters branch from 04294da to 68a578a Compare May 7, 2020 19:46

MarinPostma requested a review from Kerollmops May 7, 2020 19:52

Kerollmops requested changes May 11, 2020

View reviewed changes

Kerollmops reviewed May 11, 2020

View reviewed changes

Kerollmops approved these changes May 11, 2020

View reviewed changes

Kerollmops requested changes May 11, 2020

View reviewed changes

MarinPostma force-pushed the facet-filters branch 3 times, most recently from 8c86b6c to 7c65d48 Compare May 11, 2020 13:59

MarinPostma force-pushed the facet-filters branch from 7c65d48 to 2f71f64 Compare May 11, 2020 14:10

MarinPostma added 2 commits May 11, 2020 16:12

setting up facets

59c67f6

facet settings

270c7b0

MarinPostma force-pushed the facet-filters branch from 2f71f64 to 0e75aef Compare May 11, 2020 14:14

Kerollmops requested changes May 11, 2020

View reviewed changes

MarinPostma added 4 commits May 11, 2020 17:38

document update

e07fe01

enables faceted search

d7f099d

tests

0ce2666

update changelog

44ae216

MarinPostma force-pushed the facet-filters branch from 0e75aef to 44ae216 Compare May 11, 2020 15:49

Kerollmops approved these changes May 11, 2020

View reviewed changes

Kerollmops merged commit b215e9e into meilisearch:master May 11, 2020

MarinPostma mentioned this pull request May 13, 2020

Query with filter on nested object field returns empty hits #657

Closed

bidoubiwa mentioned this pull request May 27, 2020

Prepare documentation for v.0.11 of MeiliSearch meilisearch/documentation#379

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Facet filters #631

Facet filters #631

MarinPostma commented Apr 23, 2020

MarinPostma commented May 5, 2020

Kerollmops May 7, 2020

MarinPostma May 7, 2020

MarinPostma commented May 11, 2020

MarinPostma commented May 11, 2020

Kerollmops May 11, 2020

Kerollmops May 11, 2020

Kerollmops May 11, 2020

Kerollmops left a comment

Kerollmops May 11, 2020

Kerollmops May 11, 2020

	bad_value => Err(FacetError::unexpected_token(&["String"], bad_value)),
	bad_value => Err(FacetError::unexpected_token(&["Array"], bad_value)),

Facet filters #631

Facet filters #631

Conversation

MarinPostma commented Apr 23, 2020

MarinPostma commented May 5, 2020

Kerollmops May 7, 2020

Choose a reason for hiding this comment

MarinPostma May 7, 2020

Choose a reason for hiding this comment

MarinPostma commented May 11, 2020

MarinPostma commented May 11, 2020

Kerollmops May 11, 2020

Choose a reason for hiding this comment

Kerollmops May 11, 2020

Choose a reason for hiding this comment

Kerollmops May 11, 2020

Choose a reason for hiding this comment

Kerollmops left a comment

Choose a reason for hiding this comment

Kerollmops May 11, 2020

Choose a reason for hiding this comment

Kerollmops May 11, 2020

Choose a reason for hiding this comment