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.

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

Federated search added #567

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

Federated search added #567

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
bc3a5d6
Add federated search
Nymuxyzo Oct 11, 2024
055f7d0
Refactor federated search
ellnix Jan 30, 2025
01a293b
Add federated search code sample
ellnix Jan 30, 2025
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
5 changes: 5 additions & 0 deletions .code-samples.meilisearch.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,3 +712,8 @@ distinct_attribute_guide_distinct_parameter_1: |-
client.index('products').search('white shirt', {
distinct: 'sku'
})
multi_search_federated_1: |-
client.multi_search(
queries: [{ index_uid: 'movies', q: 'batman' }, { index_uid: 'comics', q: 'batman' }],
federation: {}
)
16 changes: 13 additions & 3 deletions lib/meilisearch/multi_search.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,20 @@

module Meilisearch
module MultiSearch
def multi_search(data)
body = Utils.transform_attributes(data)
# Performs search on one or more indexes
#
# @param [Hash] federation_options
# - `limit`: number of results in the merged list
# - `offset`: number of results to skip in the merged list
def multi_search(data = nil, queries: [], federation: nil)
Utils.soft_deprecate('multi_search([])', 'multi_search(queries: [])') if data

http_post '/multi-search', queries: body
queries += data if data

queries = Utils.transform_attributes(queries)
federation = Utils.transform_attributes(federation)

http_post '/multi-search', queries: queries, federation: federation
end
end
end
61 changes: 60 additions & 1 deletion spec/meilisearch/client/multi_search_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
end

it 'does a custom search with two different indexes' do
response = client.multi_search([
response = client.multi_search(queries: [
{ index_uid: 'books', q: 'prince' },
{ index_uid: 'movies', q: 'prince' }
])
Expand All @@ -16,4 +16,63 @@
expect(response['results'][0]['estimatedTotalHits']).to eq(0)
expect(response['results'][1]['estimatedTotalHits']).to eq(0)
end

context 'when passed a positional argument' do
before { allow(Meilisearch::Utils).to receive(:soft_deprecate).and_return(nil) }

it 'does a custom search with two different indexes' do
response = client.multi_search([
{ index_uid: 'books', q: 'prince' },
{ index_uid: 'movies', q: 'prince' }
])

expect(response['results'].count).to eq(2)
expect(response['results'][0]['estimatedTotalHits']).to eq(0)
expect(response['results'][1]['estimatedTotalHits']).to eq(0)
end

it 'warns about deprecation' do
client.multi_search([
{ index_uid: 'books', q: 'prince' },
{ index_uid: 'movies', q: 'prince' }
])

expect(Meilisearch::Utils).to have_received(:soft_deprecate)
.with('multi_search([])', a_string_including('queries'))
end
end

it 'does a federated search with two different indexes' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this test is sufficient to see that the server has indeed understood that this is a federated search and not a regular multi-search, there should be separate tests to see if the server returns what you would expect under these circumstances.

In addition we should check that limit and offset are sent properly and work as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that meet your expectations?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, from what I understood here @Nymuxyzo you would need to make two tests:

  • one asserting the inexistence of the presence of the param federation in the response
  • one asserting that you explicitly called the multi_search with a federation request, since the doc says a non-null value the value could be multi_search(data, federation: true, federation_options: {....}) # where federation options are not required

https://www.meilisearch.com/docs/learn/multi_search/multi_search_vs_federated_search#what-is-federated-search

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brunoocasali I'm sorry, but I don't understand the statement. When I look at the added test in the PHP SDK (tests/Endpoints/MultiSearchTest.php), I recognize my pattern to a very large extent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would very much like to bring this issue to a good end.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've also read and reread

one asserting the inexistence of the presence of the param federation in the response
one asserting that you explicitly called the multi_search with a federation request, since the doc says a non-null value the value could be multi_search(data, federation: true, federation_options: {....}) # where federation options are not required

and I'm sorry @brunoocasali I can't make sense of it xD

I think this might be a typo:

one asserting the inexistence of or the presence of the param federation in the response

but the presence of _federation is already looked for in the test.

Meanwhile this:

multi_search(data, federation: true, federation_options: {....}) # where federation options are not required

I'm not sure I'm following with the distinction between federation and federation_options, there is only one federation option, which can either be an object of options or a truthy value to enable it with defaults.

In any case, this should work fine as long as we don't mess with it, ruby is dynamically typed and there is nothing preventing someone from passing a boolean as federation.

Copy link
Member

@brunoocasali brunoocasali Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, @ellnix, I agree with your previous message; I will point out the change in another comment.

client.index('books').add_documents(
[
{ id: 1, title: 'Harry Potter and the Philosophers Stone' },
{ id: 2, title: 'War and Peace' },
{ id: 5, title: 'Harry Potter and the Deathly Hallows' }
]
)

client.index('movies').add_documents(
[
{ id: 1, title: 'Harry Potter and the Philosophers Stone' },
{ id: 2, title: 'Lord of the Rings' },
{ id: 4, title: 'Harry Potter and the Order of the Phoenix' }
]
).await

response = client.multi_search(queries: [
{ index_uid: 'books', q: 'Harry Potter' },
{ index_uid: 'movies', q: 'Harry Potter' }
],
federation: {
limit: 3,
offset: 1
})

expect(response['limit']).to eq(3)
expect(response['offset']).to eq(1)

hits = response['hits']
expect(hits.size).to be 3
expect(hits.first).to have_key('_federation')
end
end