Skip to content
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.

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

feat: sort by score (most relevant first) when searching #671

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

danielsaul
Copy link
Contributor

@danielsaul danielsaul commented Dec 3, 2020

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

I found when testing my search, that I was receiving strange results, with relevant ones mixed in with not so relevant ones. I spent a while digging, and found that the results are not being sorted at all.

When you perform a search, it is normally expected that the results will be sorted by the most relevant results first.
The existing search() method does not do this, and it is not possible to use sortBy() to sort by the underlying full text search score from LokiDB.

LokiDB offers a method to sortByScoring() which must be used to ensure the most relevant results are returned.

https://github.com/LokiJS-Forge/LokiDB/blob/master/packages/loki/src/result_set.ts#L377
https://github.com/LokiJS-Forge/LokiDB/blob/master/packages/full-text-search/spec/generic/full_text_search.spec.ts#L200

Therefore search() will now call sortByScoring() within it by default, sorting by the search score descending, to provide the most relevant results first.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why)

@danielsaul danielsaul changed the title add ability to sort by search score (relevancy) allow sorting by full text search score (relevancy) Dec 3, 2020
@danielsaul danielsaul changed the title allow sorting by full text search score (relevancy) feat: allow sorting by full text search score (relevancy) Dec 3, 2020
Copy link
Member

atinux commented Dec 4, 2020

Thank you for this PR @danielsaul

I think that $content().search() should actually call .sortBySearchScoring() by default to send back relevant results instead.

@danielsaul
Copy link
Contributor Author

danielsaul commented Dec 4, 2020

Thank you for this PR @danielsaul

I think that $content().search() should actually call .sortBySearchScoring() by default to send back relevant results instead.

I did consider this and is exactly what I did initially, so happy to swap back to that.

The reason I didn't was so that we could expose the direction parameter, and allow sorting by the score in ascending order. It's not really possible to add direction as an additional parameter to search().

Having said that, not sure why you would ever want to sort by score ascending 🤷 so maybe not a concern.

I'll also double-check that sortByScoring won't have adverse affects on chaining a subsequent sortBy for example, but I can't imagine it would.

Copy link
Member

atinux commented Dec 4, 2020

Having said that, not sure why you would ever want to sort by score ascending 🤷 so maybe not a concern.

Exactly, sometimes the best option is no option 😄 And anyway they can always .reverse() the result if they want to.

@danielsaul danielsaul changed the title feat: allow sorting by full text search score (relevancy) feat: sort by score (most relevant first) when searching Dec 4, 2020
@danielsaul
Copy link
Contributor Author

Having said that, not sure why you would ever want to sort by score ascending 🤷 so maybe not a concern.

Exactly, sometimes the best option is no option 😄 And anyway they can always .reverse() the result if they want to.

Cool! Simplified and updated PR description 👍
Don't think there's really any need for any documentation changes now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants