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 nearvector and neartext to hybrid search #4462

Merged
merged 25 commits into from
Mar 25, 2024
Merged

Conversation

donomii
Copy link
Contributor

@donomii donomii commented Mar 13, 2024

What's being changed:

This patch adds subsearches to hybrid search.

The basic hybrid search runs subsearches with default options. Rather than adding new and complicated ways to customise these subsearches, this patch allows users to directly specify the subsearches, using the same options as the existing nearvector and neartext searches.

To do this, I introduce two new options to hybrid search, NearText: {} and NearVector: {} . These subsearches use exactly the same options and perform exactly the same search as the currently existing searches. The results are then used as usual in hybrid.

In order to do this, I separate the code to generate a http response from the search functions. The search functions can now be used by other code, and have the return type []search.Result. There is now a function called searchResultsToGetResponse, which prepares the http results.

Clients will have to be adjusted to provide access to these subsearches. It would be good if we can share the existing code, so that adding a new feature will appear in hybrid and regular search.

The current, existing hybrid queries continue unchanged

Review checklist

  • Documentation has been updated, if necessary. Link to changed documentation:
  • Chaos pipeline run or not necessary. Link to pipeline:
  • All new code is covered by tests where it is reasonable.
  • Performance tests have been run or not necessary.

@donomii donomii self-assigned this Mar 13, 2024
@donomii donomii requested a review from a team March 13, 2024 10:13
@donomii donomii mentioned this pull request Mar 13, 2024
1 task
usecases/traverser/explorer_hybrid.go Outdated Show resolved Hide resolved
usecases/traverser/explorer_hybrid.go Show resolved Hide resolved
usecases/traverser/explorer_hybrid.go Show resolved Hide resolved
usecases/traverser/explorer_hybrid.go Show resolved Hide resolved
usecases/traverser/explorer_hybrid.go Show resolved Hide resolved
usecases/traverser/explorer_hybrid.go Show resolved Hide resolved
usecases/traverser/hybrid/searcher.go Show resolved Hide resolved
Copy link

sonarcloud bot commented Mar 22, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
11.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link
Contributor

@dirkkul dirkkul left a comment

Choose a reason for hiding this comment

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

In general I am happy to merge it, but I think it needs a few more tests:

  • invalid combinations, making sure that errors are returned correctly
  • combinations of query with searches

@donomii donomii merged commit a77d3a1 into master Mar 25, 2024
35 of 37 checks passed
@donomii donomii deleted the add-nearvector-neartext branch March 25, 2024 10:50
@Techie5879
Copy link

This seems to have been merged. @donomii Has the documentation been updated/how can I use it?

@graphinglife
Copy link

e been merged. @donomii Has the documentation been updated/how can I us

Is there documentation we can see?

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.

5 participants