-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Support using the semantic query across multiple inference IDs #133675
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 using the semantic query across multiple inference IDs #133675
Conversation
Pinging @elastic/search-relevance (Team:Search - Relevance) |
Hi @Mikep86, I've created a changelog YAML for you. |
.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me!
It looks like there are some failing tests that should be cleaned up, and I'd like to see a followup PR that removes the artificial restriction in interceptors.
Otherwise, nice work! 🙌
...erence/src/main/java/org/elasticsearch/xpack/inference/queries/InferenceResultsProvider.java
Outdated
Show resolved
Hide resolved
...nce/src/main/java/org/elasticsearch/xpack/inference/queries/MapInferenceResultsProvider.java
Outdated
Show resolved
Hide resolved
.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java
Show resolved
Hide resolved
.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/xpack/inference/queries/SingleInferenceResultsProvider.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice @Mikep86! I left one comment regarding the need of the new writables, a plain map would make things easier to read and would avoid all the boiler plates of these interfaces.
...erence/src/main/java/org/elasticsearch/xpack/inference/queries/InferenceResultsProvider.java
Outdated
Show resolved
Hide resolved
.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java
Show resolved
Hide resolved
...esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/SemanticMatchTestCase.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@elasticmachine Update branch |
@elasticmachine update branch |
@elasticmachine update branch |
merge conflict between base and head |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work!
This PR updates the
semantic
query to support querying multiple inference IDs, which can happen when queryingsemantic_text
fields across multiple indices. This is enabled by transferring inference results between coordinating and data nodes using a map keyed on inference ID.This change also benefits
match
query support since such queries are rewritten tosemantic
queries when asemantic_text
field is targeted. With this change, it should be possible to issue a multi-indexmatch
query againstsemantic_text
fields that use multiple inference IDs.