Skip to content

[DISCO-3505] remove fakespot support for suggest #7277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tiftran wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[DISCO-3505] remove fakespot support for suggest #7277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2eaaef
[DISCO-3505] remove fakespot support for suggest
tiftran Mar 18, 2026
43807a5
benchmark changes
tiftran Mar 18, 2026
ef7d48f
Merge branch 'main' into fakespot-remove
tiftran Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions components/suggest/src/benchmarks/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,6 @@ impl RemoteSettingsBenchmarkClient {
})?,
rs::Collection::Other,
)?;
new_benchmark_client.fetch_data_with_client(
remote_settings::RemoteSettings::new(remote_settings::RemoteSettingsConfig {
server: None,
bucket_name: None,
collection_name: rs::Collection::Fakespot.name().to_owned(),
server_url: None,
})?,
rs::Collection::Fakespot,
)?;
Ok(new_benchmark_client)
}

Expand Down
10 changes: 0 additions & 10 deletions components/suggest/src/benchmarks/ingest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +172,6 @@ pub fn all_benchmarks() -> Vec<(&'static str, IngestBenchmark)> {
true,
),
),
(
"ingest-fakespot",
IngestBenchmark::new(SuggestionProvider::Fakespot, false),
),
(
"ingest-again-fakespot",
IngestBenchmark::new(SuggestionProvider::Fakespot, true),
),
]
}

Expand All @@ -191,8 +183,6 @@ pub fn print_debug_ingestion_sizes() {
);
store
.ingest(SuggestIngestionConstraints {
// Uncomment to measure the size for a specific provider
// providers: Some(vec![crate::SuggestionProvider::Fakespot]),
..SuggestIngestionConstraints::default()
})
.unwrap();
Expand Down
52 changes: 0 additions & 52 deletions components/suggest/src/benchmarks/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,59 +58,7 @@ impl BenchmarkWithInput for QueryBenchmark {

pub fn all_benchmarks() -> Vec<(&'static str, QueryBenchmark)> {
vec![
// Fakespot queries, these attempt to perform prefix matches with various character
// lengths.
//
// The query code will only do a prefix match if the total input length is > 3 chars.
// Therefore, to test shorter prefixes we use 2-term queries.
(
"query-fakespot-hand-s",
QueryBenchmark {
provider: SuggestionProvider::Fakespot,
query: "hand s",
should_match: true,
}
),
(
"query-fakespot-hand-sa",
QueryBenchmark {
provider: SuggestionProvider::Fakespot,
query: "hand sa",
should_match: true,
}
),
(
"query-fakespot-hand-san",
QueryBenchmark {
provider: SuggestionProvider::Fakespot,
query: "hand san",
should_match: true,
}
),
(
"query-fakespot-sani",
QueryBenchmark {
provider: SuggestionProvider::Fakespot,
query: "sani",
should_match: true,
}
),
(
"query-fakespot-sanit",
QueryBenchmark {
provider: SuggestionProvider::Fakespot,
query: "sanit",
should_match: true,
}
),
(
"query-fakespot-saniti",
QueryBenchmark {
provider: SuggestionProvider::Fakespot,
query: "saniti",
should_match: false,
},
),

// weather: no matches
(
Expand Down
186 changes: 2 additions & 184 deletions components/suggest/src/db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,13 @@ use sql_support::{open_database, repeat_sql_vars, ConnExt};
use crate::{
config::{SuggestGlobalConfig, SuggestProviderConfig},
error::RusqliteResultExt,
fakespot,
geoname::GeonameCache,
provider::{AmpMatchingStrategy, SuggestionProvider},
query::{full_keywords_to_fts_content, FtsQuery},
rs::{
DownloadedAmoSuggestion, DownloadedAmpSuggestion, DownloadedDynamicRecord,
DownloadedDynamicSuggestion, DownloadedFakespotSuggestion, DownloadedMdnSuggestion,
DownloadedWikipediaSuggestion, Record, SuggestRecordId, SuggestRecordType,
DownloadedDynamicSuggestion, DownloadedMdnSuggestion, DownloadedWikipediaSuggestion,
Record, SuggestRecordId, SuggestRecordType,
},
schema::{clear_database, SuggestConnectionInitializer},
suggestion::{cook_raw_suggestion_url, FtsMatchInfo, Suggestion},
Expand Down Expand Up @@ -762,113 +761,6 @@ impl<'a> SuggestDao<'a> {
Ok(suggestions)
}

/// Fetches Fakespot suggestions
pub fn fetch_fakespot_suggestions(&self, query: &SuggestionQuery) -> Result<Vec<Suggestion>> {
let fts_query = query.fts_query();
let sql = r#"
SELECT
s.id,
s.title,
s.url,
s.score,
f.fakespot_grade,
f.product_id,
f.rating,
f.total_reviews,
i.data,
i.mimetype,
f.keywords,
f.product_type
FROM
suggestions s
JOIN
fakespot_fts fts
ON fts.rowid = s.id
JOIN
fakespot_custom_details f
ON f.suggestion_id = s.id
LEFT JOIN
icons i
ON i.id = f.icon_id
WHERE
fakespot_fts MATCH ?
ORDER BY
s.score DESC
"#
.to_string();

// Store the list of results plus the suggestion id for calculating the FTS match info
let mut results =
self.conn
.query_rows_and_then_cached(&sql, (&fts_query.match_arg,), |row| {
let id: usize = row.get(0)?;
let score = fakespot::FakespotScore::new(
&query.keyword,
row.get(10)?,
row.get(11)?,
row.get(3)?,
)
.as_suggest_score();
Result::Ok((
Suggestion::Fakespot {
title: row.get(1)?,
url: row.get(2)?,
score,
fakespot_grade: row.get(4)?,
product_id: row.get(5)?,
rating: row.get(6)?,
total_reviews: row.get(7)?,
icon: row.get(8)?,
icon_mimetype: row.get(9)?,
match_info: None,
},
id,
))
})?;
// Sort the results, then add the FTS match info to the first one
// For performance reasons, this is only calculated for the result with the highest score.
// We assume that only one that will be shown to the user and therefore the only one we'll
// collect metrics for.
results.sort();
if let Some((suggestion, id)) = results.first_mut() {
match suggestion {
Suggestion::Fakespot {
match_info, title, ..
} => {
*match_info = Some(self.fetch_fakespot_fts_match_info(&fts_query, *id, title)?);
}
_ => unreachable!(),
}
}
Ok(results
.into_iter()
.map(|(suggestion, _)| suggestion)
.collect())
}

fn fetch_fakespot_fts_match_info(
&self,
fts_query: &FtsQuery<'_>,
suggestion_id: usize,
title: &str,
) -> Result<FtsMatchInfo> {
let prefix = if fts_query.is_prefix_query {
// If the query was a prefix match query then test if the query without the prefix
// match would have also matched. If not, then this counts as a prefix match.
let sql = "SELECT 1 FROM fakespot_fts WHERE rowid = ? AND fakespot_fts MATCH ?";
let params = (&suggestion_id, &fts_query.match_arg_without_prefix_match);
!self.conn.exists(sql, params)?
} else {
// If not, then it definitely wasn't a prefix match
false
};

Ok(FtsMatchInfo {
prefix,
stemming: fts_query.match_required_stemming(title),
})
}

/// Fetches dynamic suggestions
pub fn fetch_dynamic_suggestions(&self, query: &SuggestionQuery) -> Result<Vec<Suggestion>> {
let Some(suggestion_types) = query
Expand Down Expand Up @@ -1114,27 +1006,6 @@ impl<'a> SuggestDao<'a> {
Ok(())
}

/// Inserts all suggestions from a downloaded Fakespot attachment into the database.
pub fn insert_fakespot_suggestions(
&mut self,
record_id: &SuggestRecordId,
suggestions: &[DownloadedFakespotSuggestion],
) -> Result<()> {
let mut suggestion_insert = SuggestionInsertStatement::new(self.conn)?;
let mut fakespot_insert = FakespotInsertStatement::new(self.conn)?;
for suggestion in suggestions {
let suggestion_id = suggestion_insert.execute(
record_id,
&suggestion.title,
&suggestion.url,
suggestion.score,
SuggestionProvider::Fakespot,
)?;
fakespot_insert.execute(suggestion_id, suggestion)?;
}
Ok(())
}

/// Inserts dynamic suggestion records data into the database.
pub fn insert_dynamic_suggestions(
&mut self,
Expand Down Expand Up @@ -1295,14 +1166,6 @@ impl<'a> SuggestDao<'a> {
named_params! { ":record_id": record_id.as_str() },
)?;
self.scope.err_if_interrupted()?;
self.conn.execute_cached(
"
DELETE FROM fakespot_fts
WHERE rowid IN (SELECT id from suggestions WHERE record_id = :record_id)
",
named_params! { ":record_id": record_id.as_str() },
)?;
self.scope.err_if_interrupted()?;
self.conn.execute_cached(
"DELETE FROM suggestions WHERE record_id = :record_id",
named_params! { ":record_id": record_id.as_str() },
Expand Down Expand Up @@ -1677,51 +1540,6 @@ impl<'conn> MdnInsertStatement<'conn> {
}
}

struct FakespotInsertStatement<'conn>(rusqlite::Statement<'conn>);

impl<'conn> FakespotInsertStatement<'conn> {
fn new(conn: &'conn Connection) -> Result<Self> {
Ok(Self(conn.prepare(
"INSERT INTO fakespot_custom_details(
suggestion_id,
fakespot_grade,
product_id,
keywords,
product_type,
rating,
total_reviews,
icon_id
)
VALUES(?, ?, ?, ?, ?, ?, ?, ?)
",
)?))
}

fn execute(
&mut self,
suggestion_id: i64,
fakespot: &DownloadedFakespotSuggestion,
) -> Result<()> {
let icon_id = fakespot
.product_id
.split_once('-')
.map(|(vendor, _)| format!("fakespot-{vendor}"));
self.0
.execute((
suggestion_id,
&fakespot.fakespot_grade,
&fakespot.product_id,
&fakespot.keywords.to_lowercase(),
&fakespot.product_type.to_lowercase(),
fakespot.rating,
fakespot.total_reviews,
icon_id,
))
.with_context("fakespot insert")?;
Ok(())
}
}

struct DynamicInsertStatement<'conn>(rusqlite::Statement<'conn>);

impl<'conn> DynamicInsertStatement<'conn> {
Expand Down
Loading