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

Add support for search ranking score #413

Merged
merged 4 commits into from
Sep 27, 2023

Conversation

Sherlouk
Copy link
Collaborator

@Sherlouk Sherlouk commented Sep 17, 2023

Pull Request

Related issue

Fixes #398

What does this PR do?

  • Add the ability receive a new param in the search method called showRankingScore.
  • Add the ability handle the new _rankingScore key/value in the search hits' response.
  • Add integration tests
  • Update the code-samples accordingly

⚠️ This is a breaking change.

I'd love to hear some thoughts from others on how we may be able to avoid this kind of breaking change, but with #398 and #399 new values are being added to the "hit" object. The hit object is currently a generic provided by the user of the library, and as such it's impossible for us to add new attributes to that.

In this PR I've tried to get around this by creating our own encapsulation called "SearchHit" which itself holds a reference to the actual result but also any additional attributes that Meilisearch itself returns. In order to minimise the impact on users of the library I have implemented a "dynamic member lookup" with a keypath, this continues to support (in a completely type safe and compiler safe way) dot notation to variables within the hit result. Which means hits[0].title would still return the title on the hit, even though architecturally it's address is hits[0].result.title. While this does work for the vast majority of use cases, we are unable to support the case where a user tries to cast a hit to a type, you can see this in our tests where you can no longer cast hits[0] to a "Book", rather it is now a "SearchHit", but again... you can access variables the same and no other changes are needed to the code.

Importantly though this workaround with dynamic member lookups is limited to the capabilities of keypaths, which as documented officially by Apple here does not include methods. As such where users have functions within their models, they would have to call that through hits[0].result.someFunction().

I do personally think this is the best compromise. It is a breaking change, but most will not be impacted in any meaningful way. If others have a better idea though I'd love to hear it. This PR lays up the foundations for #399 which would be significantly easier once this is merged. But let's discuss.

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@curquiza
Copy link
Member

@Sherlouk thank for your PR
Multiple points

  • There are now git conflict, can you fix them? 😄
  • Can you detail what your breaking is to have more context: what the users had to do before, and what should they do now?
  • You have to know this SDK has really a low usage, so we can allow breaking. Of course we do our best to avoid them, especially if it concerns the main features (search and add documents)

@curquiza curquiza added enhancement New feature or request breaking-change The related changes are breaking for the users labels Sep 21, 2023
@Sherlouk
Copy link
Collaborator Author

Sherlouk commented Sep 21, 2023

Rebase completed, tests updated, and all running locally.

Breaking Change

This is a breaking change affecting the results returned by use of the search(_:) function.

Previously, you would get back a response of Searchable<T> where "T" is a generic type representing the model of the document stored in MeiliSearch. Searchable includes an array called "hits" - the search results: an array of documents matching the query parameters.

With this PR, instead of "hits" being an array of documents, it is now an array of SearchHit<T>'s. You must now access the document contents by using hits[0].document instead of simply hits[0]. This encapsulating type allows us to add metadata unique to the hit but that is not part of the document itself (including the ranking score, ranking stats, match positions, formatted data, and any future search API expansion - without more breaking changes).

For convenience, this PR includes a dynamic member lookup. This isn't strictly necessary, but it does reduce (and in many cases completely avoids) any errors caused by this PR. It has some limitations, which I'll explain with some code snippets:

let result: Searchable<Book> = ... // unchanged

// accessing members (variables) on the document continues to work without change, thanks to the dynamic member lookup
print(result.hits[0].title) // VALID

// ---

// the result hit is no longer an instance of "T"
let book: Book = result.hits[0] // NEW ERROR
print(book.title)

let book: SearchHit<Book> = result.hits[0] // FIXED
print(book.title)

let book = result.hits[0] // ALSO FIXES ERROR
print(book.title)

let book: Book = result.hits[0].document // ALSO FIXES ERROR
print(book.title)

// ---

// functions do not work with dynamic member lookup
result.hits[0].getStock() // NEW ERROR
result.hits[0].document.getStock() // WITH FIX

Worst case, all errors caused by this PR simply need the developer to add .document to their usage as seen with the last example above. Best case, it continues to work with no changes necessary thanks to the dynamic member lookup.

@curquiza
Copy link
Member

Thank you @Sherlouk
Following this breaking, do we need to update some examples the README or in the code samples file like this one (getting_started_add_documents_md)?

@Sherlouk
Copy link
Collaborator Author

No documentation currently references the code which is changed by this PR, so no updates needed.

@Sherlouk
Copy link
Collaborator Author

It's worth noting (though not necessarily required for this PR) that I do plan on doing a full review of code samples and documentation to improve adoption of the project. There are others that are outdated (but none because of this PR that I can see).

@curquiza
Copy link
Member

It's worth noting (though not necessarily required for this PR) that I do plan on doing a full review of code samples and documentation to improve adoption of the project. There are others that are outdated (but none because of this PR that I can see).

Good to know! I will open an issue regarding it: I have scripts to know which code samples are missing and which ones should be removed 😊

@curquiza curquiza added enhancement New feature or request and removed enhancement New feature or request labels Sep 27, 2023
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Sep 27, 2023

@meili-bors meili-bors bot merged commit e3ba51a into meilisearch:main Sep 27, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v1.3] Display hits ranking scores
2 participants