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

Support collections in multi_search #405

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

Conversation

ellnix
Copy link
Collaborator

@ellnix ellnix commented Mar 13, 2025

Pull Request

Related issue

Fixes part of #397

  • This makes multi_search, like the regular search, support relations and other collections that respond to where.
  • In favor of this new :collection option, :class_name is softly deprecated for a consistent user experience (since :collection does everything that :class_name did and more)

Copy link

codecov bot commented Mar 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (5450df2) to head (d969d9b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #405      +/-   ##
==========================================
+ Coverage   89.54%   89.61%   +0.06%     
==========================================
  Files          13       13              
  Lines         775      780       +5     
==========================================
+ Hits          694      699       +5     
  Misses         81       81              

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ellnix
Copy link
Collaborator Author

ellnix commented Mar 13, 2025

@pozelli Please review and see if this makes multi_search behave how you expect it to 😄

@wesharper If you are interested in doing code review please take a look at this PR. (if you are not interested that's OK, it's not an assignment 😆 think of it more like a +cc)

@ellnix ellnix force-pushed the multi_search_collections branch from 93343f0 to f7dcae3 Compare March 14, 2025 19:19
@ellnix ellnix force-pushed the multi_search_collections branch from f7dcae3 to d969d9b Compare March 14, 2025 19:26
@pozelli
Copy link

pozelli commented Mar 14, 2025

@pozelli Please review and see if this makes multi_search behave how you expect it to 😄

Sure.

This PR addresses these items from #397:

Currently, multi_search (e.g., in FederatedSearchResult) does not seem to support queries on collections. It only works with models or simple indexes.

In a federated search, since it is not possible to use collections but only models or simple indexes, I must first perform an additional step: Queries to retrieve the IDs of each record.

This extra query could already be avoided if federated search supported collections, just as simple search does.

On the other hand:

However, we would still face the pagination issue.

I have tested the PR, and it works as I expected it to.

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.

2 participants