Merge #318

318: Revert "Sort at query time" r=Kerollmops a=curquiza

Reverts #309

We revert this from `main` not because this leads to a bug, but because we don't want to release it now and we have to merge and release an hotfix on `main`.
Cf:
- #316
- #317

Once the v0.21.0 is released, we should merge again this awesome addition 👌 

Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>

Author: bors[bot]

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<String> = 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
         },
 

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,

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"] }

milli/src/criterion.rs

@@ -1,24 +1,26 @@
 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<Regex> =
+    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.
     /// Query words at the front of an attribute is considered better than if it was at the back.
     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<Criterion, Self::Err> {
-        match text {
+    fn from_str(txt: &str) -> Result<Criterion, Self::Err> {
+        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<AscDesc, Self::Err> {
-        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<Criterion> {
     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),
         }
     }
 }

milli/src/error.rs

@@ -58,7 +58,6 @@ pub enum UserError {
     InvalidFacetsDistribution { invalid_facets_name: HashSet<String> },
     InvalidFilter(pest::error::Error<ParserRule>),
     InvalidFilterAttribute(pest::error::Error<ParserRule>),
-    InvalidSortableAttribute { field: String, valid_fields: HashSet<String> },
     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::<Vec<_>>().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)

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<String>,
-    ) -> 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<bool> {
-        self.main.delete::<_, Str>(wtxn, main_key::SORTABLE_FIELDS_KEY)
-    }
-
-    /// Returns the sortable fields names.
-    pub fn sortable_fields(&self, rtxn: &RoTxn) -> heed::Result<HashSet<String>> {
-        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<HashSet<FieldId>> {
-        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<HashSet<String>> {
         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());

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,
 };