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 multi-search federation #563

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Conversation

polyfloyd
Copy link
Contributor

@polyfloyd polyfloyd commented Aug 20, 2024

Pull Request

Fixes #573

What does this PR do?

This PR adds support for the new federated search to be introduced in v1.10: https://github.com/meilisearch/meilisearch/releases/tag/v1.10.0-rc.0

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?

@polyfloyd
Copy link
Contributor Author

Please note that I am running into an issue here: unaccepted status code found: 400 expected: [200], MeilisearchApiError Message: Inside .queries[0]: Using pagination options is not allowed in federated queries.

I think this patch is correct, so I'm opening this PR already. But I can not be 100% sure just yet

@curquiza
Copy link
Member

Hello @polyfloyd
thanks for your PR, but v1.10 is not released yet. So I will review and merge a PR for federated once v1.10 is release 😊

Thanks for your anticipation and involvement 😄

@curquiza curquiza added the enhancement New feature or request label Aug 20, 2024
@polyfloyd
Copy link
Contributor Author

Found the cause of the error message, this Go library sets the pagination limit of each query to a default. I removed it for now so I can continue testing

@Ja7ad
Copy link
Collaborator

Ja7ad commented Aug 20, 2024

@polyfloyd Thank you, currently you can change this PR to draft until release v1.10 and add issue for federation search.

@polyfloyd
Copy link
Contributor Author

Saw that there is an issue open now: #573

Will go over the checklist to see whether there is still something to be done

@Ja7ad
Copy link
Collaborator

Ja7ad commented Aug 28, 2024

Saw that there is an issue open now: #573

Will go over the checklist to see whether there is still something to be done

Please do this check list:

  • write test stage for multi-search with federation
  • please add fixes #573 in PR description
  • add _federation for SearchResponse
"_federation": {
        "indexUid": "comics",
        "queriesPosition": 1
      }

@polyfloyd
Copy link
Contributor Author

Thanks!

types.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
@polyfloyd polyfloyd force-pushed the add-federation branch 2 times, most recently from 97600c4 to 456b1a6 Compare August 28, 2024 15:21
@polyfloyd
Copy link
Contributor Author

Updated the commit title and added a test.

Adding _federation to SearchResponse does not seem correct? This object is present in each item of the hits array. See the expected payload of the new unit test to see what I mean

meilisearch_test.go Outdated Show resolved Hide resolved
meilisearch_test.go Outdated Show resolved Hide resolved
@Ja7ad
Copy link
Collaborator

Ja7ad commented Aug 28, 2024

Updated the commit title and added a test.

Adding _federation to SearchResponse does not seem correct? This object is present in each item of the hits array. See the expected payload of the new unit test to see what I mean

Don't require any do for this.

@Ja7ad
Copy link
Collaborator

Ja7ad commented Aug 29, 2024

@polyfloyd please add fixes #573 in PR description

@polyfloyd
Copy link
Contributor Author

@Ja7ad I have added the issue ref to the PR description

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.72%. Comparing base (e09d86e) to head (d132bc1).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #563      +/-   ##
==========================================
- Coverage   86.73%   86.72%   -0.02%     
==========================================
  Files          11       11              
  Lines        2043     2041       -2     
==========================================
- Hits         1772     1770       -2     
  Misses        165      165              
  Partials      106      106              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Ja7ad
Copy link
Collaborator

Ja7ad commented Sep 18, 2024

bors merge

Thank you!!

Copy link
Contributor

meili-bors bot commented Sep 18, 2024

@polyfloyd polyfloyd closed this Sep 18, 2024
@meili-bors meili-bors bot merged commit 63161f2 into meilisearch:main Sep 18, 2024
6 checks passed
@polyfloyd polyfloyd deleted the add-federation branch September 18, 2024 15:26
meili-bors bot added a commit that referenced this pull request Oct 28, 2024
584: Update version for the next release (v0.29.0) r=brunoocasali a=meili-bot

_This PR is auto-generated._

The automated script updates the version of meilisearch-go to a new version: "v0.29.0"

--- Changelogs 👇 ---

## ⚠️ Breaking changes (experimental feature)

* `embedder` is now mandatory everywhere - Ensure compatibility with [Meilisearch v1.11](https://github.com/meilisearch/meilisearch/releases/tag/v1.11.0) by `@/Kerollmops`

## 🚀 Enhancements

* Add Embedder.URL (#568) `@/polyfloyd`
* Add multi-search federation (#563) `@/polyfloyd`
* Feat support content encoding  (#570) `@/Ja7ad`
* Support experimental manager (#572) `@/A7bari`
* Feat Language settings & search parameter (#580) `@J/a7ad`
* Feat support retry pattern on status code 502, 503, 504 (#581) `@/Ja7ad`

Thanks again to `@/A7bari,` `@/Ja7ad,` `@/Kerollmops`  and `@/polyfloyd!` 🎉



Co-authored-by: meili-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v1.10.0] Federated search
3 participants