Bug 1931373 - Add FTS matching data #6531
Conversation
|
I think this is working based on running some tests with the CLI: I used variations on "running shoe" to check the stemming/prefix matching flags: I used these queries to test the term distance: |
bendk
force-pushed
the
push-mollqwzklyms
branch
from
474496c to
1ffb901
Compare
| // All terms in a 5-term chunk | ||
| Medium, | ||
| // No 5-term chunk that contains all the terms | ||
| Far, |
There was a problem hiding this comment.
The segments above were arbitrarily picked by me. Are there different numbers we should choose?
There was a problem hiding this comment.
I don't know, what's the intuition behind collecting this distance data?
3 seems kind of like a large distance to start with. If I'm reading the fts5 doc right, that means terms can have 3 words between them and the match will still be successful. I would think we'd want to start at 1 at most, maybe even 0?
Is there a reason for using an enum and not reporting the numeric distance itself, as like min_term_distance?
How hard would it be to add tests for non-near distances?
There was a problem hiding this comment.
Good catch on this one, the docs confused me and I thought 3 meant the total amount of words in the clump was 3. I changed these numbers to 1 and 3, which seems like more of a reasonable start.
That said, I'm still not sure what's correct to test for, I'm open to changing "Near" to meaning 0.
The one thing I don't think we can do is calculate actual minimum term distance number. AFAICT, there's no function for that, you just have to make a bunch of queries and see if they match or not. We have variants like Adjacent=0, Medium=1 and Far=2 or greater though.
bendk
force-pushed
the
push-mollqwzklyms
branch
from
1ffb901 to
302ec01
Compare
|
|
||
| fn split_terms(phrase: &str) -> Vec<&str> { | ||
| phrase | ||
| .split([' ', '(', ')', ':', '^', '*', '"', ',']) |
There was a problem hiding this comment.
I added the , char in the list of things to split on. This way the comma in Trail Running, isn't included in the search terms. The FTS tokenizer ignores that, but it was messing up the stemming check logic.
bendk
force-pushed
the
push-mollqwzklyms
branch
3 times, most recently
from
33d369c to
e6880b6
Compare
There was a problem hiding this comment.
Thanks, lgtm. It's too bad several extra queries may be needed to get the match info, but I'm guessing it's not a big problem (in terms of latency at least) since they should all be using indexes? You could probably do one big query with SELECT subqueries to get all the info in one, but it's probably not worth it.
components/suggest/src/query.rs
Outdated
| // This is used when passing the keywords into an FTS search. It: | ||
| // - Strips out any `():^*"` chars. These are typically used for advanced searches, which | ||
| // we don't support and it would be weird to only support for FTS searches, which | ||
| // currently means Fakespot searches. |
There was a problem hiding this comment.
Nit: I would remove the part about Fakespot so we don't need to remember to update this when we add more FTS suggestions.
components/suggest/src/query.rs
Outdated
| pub fn sqlite_match_without_prefix_match(&self) -> &str { | ||
| self.sqlite_match | ||
| .strip_suffix('*') | ||
| .unwrap_or(&self.sqlite_match) |
There was a problem hiding this comment.
Instead of needing a method to compute this, couldn't you have a FtsQuery::sqlite_match_without_prefix_match string that you would initialize in new() as part of the prefix_match if-statement?
| // All terms in a 5-term chunk | ||
| Medium, | ||
| // No 5-term chunk that contains all the terms | ||
| Far, |
There was a problem hiding this comment.
I don't know, what's the intuition behind collecting this distance data?
3 seems kind of like a large distance to start with. If I'm reading the fts5 doc right, that means terms can have 3 words between them and the match will still be successful. I would think we'd want to start at 1 at most, maybe even 0?
Is there a reason for using an enum and not reporting the numeric distance itself, as like min_term_distance?
How hard would it be to add tests for non-near distances?
components/suggest/src/suggestion.rs
Outdated
| }, | ||
| Exposure { | ||
| suggestion_type: String, | ||
| score: f64, | ||
| }, | ||
| } | ||
|
|
||
| /// Additional data about how an FTS match was made(https://bugzilla.mozilla.org/show_bug.cgi?id=1931373) |
There was a problem hiding this comment.
Nit: Missing a space in "made(" but I'd just leave out the bug URL. If we need to find the bug/PR where this change was made, we can check blame.
Added extra data to `Suggestion::Fakespot` to capture how the FTS match was made. The plan is to use this as a facet for our metrics to help us consider how to tune the matching logic (i.e. maybe we should not use stemming, maybe we should reqiure that terms are close together). Added Suggest CLI flag to print out the FTS match info.
bendk
force-pushed
the
push-mollqwzklyms
branch
from
e6880b6 to
074c301
Compare
|
I got distracted last week, but I'm picking it back up now. Thanks for the great review, I think this one is looking much better now. |
I refactored this to use subqueries and I think it's actually cleaner this way, so let's go for it. |
Added extra data to
Suggestion::Fakespotto capture how the FTS match was made. The plan is to use this as a facet for our metrics to help us consider how to tune the matching logic (i.e. maybe we should not use stemming, maybe we should reqiure that terms are close together).Added Suggest CLI flag to print out the full debug repr for suggestions. This provides an easy way to test the new functionality.
Pull Request checklist
[ci full]to the PR title.Branch builds: add
[firefox-android: branch-name]to the PR title.