Bug 1957309: Handle location signs as one of Yelp modifier (again)

Author: dadaa

components/suggest/src/db.rs

@@ -1355,11 +1355,6 @@ impl<'a> SuggestDao<'a> {
             named_params! { ":record_id": record_id.as_str() },
         )?;
         self.scope.err_if_interrupted()?;
-        self.conn.execute_cached(
-            "DELETE FROM yelp_location_signs WHERE record_id = :record_id",
-            named_params! { ":record_id": record_id.as_str() },
-        )?;
-        self.scope.err_if_interrupted()?;
         self.conn.execute_cached(
             "DELETE FROM yelp_custom_details WHERE record_id = :record_id",
             named_params! { ":record_id": record_id.as_str() },

components/suggest/src/rs.rs

@@ -41,6 +41,7 @@ use serde::{Deserialize, Deserializer, Serialize};
 use serde_json::{Map, Value};
 
 use crate::{error::Error, query::full_keywords_to_fts_content, Result};
+use rusqlite::{types::ToSqlOutput, ToSql};
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub enum Collection {
@@ -453,12 +454,21 @@ pub(crate) struct DownloadedPocketSuggestion {
     pub high_confidence_keywords: Vec<String>,
     pub score: f64,
 }
-/// A location sign for Yelp to ingest from a Yelp Attachment
+/// Yelp location sign data type
 #[derive(Clone, Debug, Deserialize)]
-pub(crate) struct DownloadedYelpLocationSign {
-    pub keyword: String,
-    #[serde(rename = "needLocation")]
-    pub need_location: bool,
+#[serde(untagged)]
+pub enum DownloadedYelpLocationSign {
+    V1 { keyword: String },
+    V2(String),
+}
+impl ToSql for DownloadedYelpLocationSign {
+    fn to_sql(&self) -> rusqlite::Result<ToSqlOutput<'_>> {
+        let keyword = match self {
+            DownloadedYelpLocationSign::V1 { keyword } => keyword,
+            DownloadedYelpLocationSign::V2(keyword) => keyword,
+        };
+        Ok(ToSqlOutput::from(keyword.as_str()))
+    }
 }
 /// A Yelp suggestion to ingest from a Yelp Attachment
 #[derive(Clone, Debug, Deserialize)]

components/suggest/src/schema.rs

@@ -23,7 +23,7 @@ use sql_support::{
 ///     `clear_database()` by adding their names to `conditional_tables`, unless
 ///     they are cleared via a deletion trigger or there's some other good
 ///     reason not to do so.
-pub const VERSION: u32 = 36;
+pub const VERSION: u32 = 37;
 
 /// The current Suggest database schema.
 pub const SQL: &str = "
@@ -171,12 +171,6 @@ CREATE TABLE yelp_modifiers(
     PRIMARY KEY (type, keyword)
 ) WITHOUT ROWID;
 
-CREATE TABLE yelp_location_signs(
-    keyword TEXT PRIMARY KEY,
-    need_location INTEGER NOT NULL,
-    record_id TEXT NOT NULL
-) WITHOUT ROWID;
-
 CREATE TABLE yelp_custom_details(
     icon_id TEXT PRIMARY KEY,
     score REAL NOT NULL,
@@ -649,6 +643,10 @@ impl ConnectionInitializer for SuggestConnectionInitializer<'_> {
                 // The commit that added this migration was reverted.
                 Ok(())
             }
+            36 => {
+                tx.execute_batch("DROP TABLE IF EXISTS yelp_location_signs;")?;
+                Ok(())
+            }
             _ => Err(open_database::Error::IncompatibleVersion(version)),
         }
     }
@@ -666,7 +664,6 @@ pub fn clear_database(db: &Connection) -> rusqlite::Result<()> {
         DELETE FROM icons;
         DELETE FROM yelp_subjects;
         DELETE FROM yelp_modifiers;
-        DELETE FROM yelp_location_signs;
         DELETE FROM yelp_custom_details;
         ",
     )?;
@@ -850,4 +847,26 @@ PRAGMA user_version=16;
 
         Ok(())
     }
+
+    /// Test that yelp_location_signs table could be removed correctly.
+    #[test]
+    fn test_remove_yelp_location_signs_table() -> anyhow::Result<()> {
+        // Start with the v16 schema.
+        let db_file =
+            MigratedDatabaseFile::new(SuggestConnectionInitializer::default(), V16_SCHEMA);
+
+        // Upgrade to v36.
+        db_file.upgrade_to(36);
+
+        // Drop the table to simulate old 35 > 36 migration.
+        let conn = db_file.open();
+        conn.execute("DROP table yelp_location_signs", ())?;
+        conn.close().expect("Connection should be closed");
+
+        // Finish upgrading to the current version.
+        db_file.upgrade_to(VERSION);
+        db_file.assert_schema_matches_new_database();
+
+        Ok(())
+    }
 }

components/suggest/src/testing/data.rs

@@ -297,10 +297,12 @@ pub fn ramen_yelp() -> JsonValue {
         "preModifiers": ["best", "super best", "same_modifier"],
         "postModifiers": ["delivery", "super delivery", "same_modifier"],
         "locationSigns": [
+            // V1 format also can be used as location sign.
             { "keyword": "in", "needLocation": true },
-            { "keyword": "near", "needLocation": true },
             { "keyword": "near by", "needLocation": false },
-            { "keyword": "near me", "needLocation": false },
+            // V2 format.
+            "near",
+            "near me",
         ],
         "yelpModifiers": ["yelp", "yelp keyword"],
         "icon": "yelp-favicon",

components/suggest/src/yelp.rs

@@ -22,6 +22,7 @@ enum Modifier {
     Pre = 0,
     Post = 1,
     Yelp = 2,
+    LocationSign = 3,
 }
 
 impl ToSql for Modifier {
@@ -53,11 +54,6 @@ const MAX_QUERY_LENGTH: usize = 150;
 /// "keyword=:modifier" (please see is_modifier()), define this how many words we should check.
 const MAX_MODIFIER_WORDS_NUMBER: usize = 2;
 
-/// The max number of words consisting the location sign. To improve the SQL performance by matching
-/// with "keyword=:modifier" (please see is_location_sign()), define this how many words we should
-/// check.
-const MAX_LOCATION_SIGN_WORDS_NUMBER: usize = 2;
-
 /// At least this many characters must be typed for a subject to be matched.
 const SUBJECT_PREFIX_MATCH_THRESHOLD: usize = 2;
 
@@ -115,14 +111,14 @@ impl SuggestDao<'_> {
             )?;
         }
 
-        for sign in &suggestion.location_signs {
+        for keyword in &suggestion.location_signs {
             self.scope.err_if_interrupted()?;
             self.conn.execute_cached(
-                "INSERT INTO yelp_location_signs(record_id, keyword, need_location) VALUES(:record_id, :keyword, :need_location)",
+                "INSERT INTO yelp_modifiers(record_id, type, keyword) VALUES(:record_id, :type, :keyword)",
                 named_params! {
                     ":record_id": record_id.as_str(),
-                    ":keyword": sign.keyword,
-                    ":need_location": sign.need_location,
+                    ":type": Modifier::LocationSign,
+                    ":keyword": keyword,
                 },
             )?;
         }
@@ -178,7 +174,8 @@ impl SuggestDao<'_> {
             query_words = rest;
         }
 
-        let location_sign_tuple = self.find_location_sign(query_words)?;
+        let location_sign_tuple =
+            self.find_modifier(query_words, Modifier::LocationSign, FindFrom::First)?;
         if let Some((_, rest)) = location_sign_tuple {
             query_words = rest;
         }
@@ -201,7 +198,6 @@ impl SuggestDao<'_> {
             subject_exact_match: subject_tuple.1,
             pre_modifier: pre_modifier_tuple.map(|(words, _)| words.to_string()),
             post_modifier: post_modifier_tuple.map(|(words, _)| words.to_string()),
-            need_location: location_sign_tuple.is_some() || location.is_some(),
             location_sign: location_sign_tuple.map(|(words, _)| words.to_string()),
             location,
             icon,
@@ -343,55 +339,6 @@ impl SuggestDao<'_> {
         Ok(None)
     }
 
-    /// Find the location sign for given query.
-    /// It returns Option<tuple> as follows:
-    /// (
-    ///   String: The keyword in DB (but the case is inherited by query).
-    ///   &[&str]: Words after removed matching location sign.
-    /// )
-    fn find_location_sign<'a>(
-        &self,
-        query_words: &'a [&'a str],
-    ) -> Result<Option<(String, &'a [&'a str])>> {
-        if query_words.is_empty() {
-            return Ok(None);
-        }
-
-        for n in (1..=std::cmp::min(MAX_LOCATION_SIGN_WORDS_NUMBER, query_words.len())).rev() {
-            let Some((candidate_chunk, rest)) = query_words.split_at_checked(n) else {
-                continue;
-            };
-
-            let mut candidate = candidate_chunk.join(" ");
-
-            if let Some(keyword_lowercase) = self.conn.try_query_one::<String, _>(
-                if n == query_words.len() {
-                    "
-                    SELECT keyword FROM yelp_location_signs
-                    WHERE keyword BETWEEN :word AND :word || x'FFFF'
-                    LIMIT 1
-                    "
-                } else {
-                    "
-                    SELECT keyword FROM yelp_location_signs
-                    WHERE keyword = :word
-                    LIMIT 1
-                    "
-                },
-                named_params! {
-                    ":word": candidate.to_lowercase(),
-                },
-                true,
-            )? {
-                // Preserve the query as the user typed it including its case.
-                candidate.push_str(keyword_lowercase.get(candidate.len()..).unwrap_or_default());
-                return Ok(Some((candidate, rest)));
-            }
-        }
-
-        Ok(None)
-    }
-
     /// Fetch the custom details for Yelp suggestions.
     /// It returns the location tuple as follows:
     /// (
@@ -440,25 +387,17 @@ struct SuggestionBuilder<'a> {
     post_modifier: Option<String>,
     location_sign: Option<String>,
     location: Option<String>,
-    need_location: bool,
     icon: Option<Vec<u8>>,
     icon_mimetype: Option<String>,
     score: f64,
 }
 
 impl<'a> From<SuggestionBuilder<'a>> for Suggestion {
     fn from(builder: SuggestionBuilder<'a>) -> Suggestion {
-        // This location sign such the 'near by' needs to add as a description parameter.
-        let location_modifier = if !builder.need_location {
-            builder.location_sign.as_deref()
-        } else {
-            None
-        };
         let description = [
             builder.pre_modifier.as_deref(),
             Some(builder.subject),
             builder.post_modifier.as_deref(),
-            location_modifier,
         ]
         .iter()
         .flatten()
@@ -470,7 +409,7 @@ impl<'a> From<SuggestionBuilder<'a>> for Suggestion {
         let mut url = String::from("https://www.yelp.com/search?");
         let mut parameters = form_urlencoded::Serializer::new(String::new());
         parameters.append_pair("find_desc", &description);
-        if let (Some(location), true) = (&builder.location, builder.need_location) {
+        if let Some(location) = &builder.location {
             parameters.append_pair("find_loc", location);
         }
         url.push_str(&parameters.finish());
@@ -494,7 +433,7 @@ impl<'a> From<SuggestionBuilder<'a>> for Suggestion {
             icon: builder.icon,
             icon_mimetype: builder.icon_mimetype,
             score: builder.score,
-            has_location_sign: location_modifier.is_none() && builder.location_sign.is_some(),
+            has_location_sign: builder.location_sign.is_some(),
             subject_exact_match: builder.subject_exact_match,
             location_param: "find_loc".to_string(),
         }
@@ -756,7 +695,11 @@ mod tests {
             ];
             for (query, expected) in find_location_sign_tests {
                 assert_eq!(
-                    dao.find_location_sign(&query.split_whitespace().collect::<Vec<_>>())?,
+                    dao.find_modifier(
+                        &query.split_whitespace().collect::<Vec<_>>(),
+                        Modifier::LocationSign,
+                        FindFrom::First
+                    )?,
                     *expected
                 );
             }