-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Add support for sparse_vector queries against semantic_text fields #118617
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 support for sparse_vector queries against semantic_text fields #118617
Conversation
Hi @kderusso, I've created a changelog YAML for you. |
56d3f46
to
c427143
Compare
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
Pinging @elastic/search-eng (Team:SearchOrg) |
Pinging @elastic/search-relevance (Team:Search - Relevance) |
Hi @kderusso, I've created a changelog YAML for you. |
...e/src/yamlRestTest/resources/rest-api-spec/test/inference/46_semantic_text_sparse_vector.yml
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.
Looks great! Love how the rewriting mechanism makes this soooo easy!
++ to Ben's comment on removing the need for inference_id
for semantic_text.
A refactoring question about introducing a base class for your consideration.
...a/org/elasticsearch/xpack/inference/queries/SemanticSparseVectorQueryRewriteInterceptor.java
Outdated
Show resolved
Hide resolved
new SparseVectorQueryBuilder( | ||
getNestedEmbeddingsField(sparseVectorQueryBuilder.getFieldName()), | ||
sparseVectorQueryBuilder.getQueryVectors(), | ||
sparseVectorQueryBuilder.getInferenceId(), |
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.
This should be retrieved from the index metadata, as per Ben's comment this is not necessary
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.
For some use cases we will still need it, for example if the request is sending in query vectors. However, I will address this
...e/src/yamlRestTest/resources/rest-api-spec/test/inference/46_semantic_text_sparse_vector.yml
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.
Great work, I think the overall mechanism for implementing this looks good. I agree with Ben and Carlos that we need to work on making inference_id
optional when querying a semantic_text
field in this PR, as it is a big part of the user experience.
.../src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryInterceptionUtils.java
Outdated
Show resolved
Hide resolved
...ain/java/org/elasticsearch/xpack/inference/queries/SemanticMatchQueryRewriteInterceptor.java
Outdated
Show resolved
Hide resolved
boolQueryBuilder.should( | ||
SemanticQueryInterceptionUtils.createSubQueryForIndices( | ||
inferenceIndexInformationForField.nonInferenceIndices(), | ||
SemanticQueryInterceptionUtils.createSubQueryForIndices( | ||
inferenceIndexInformationForField.nonInferenceIndices(), | ||
sparseVectorQueryBuilder | ||
) | ||
) | ||
); |
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.
Why nest two sub-queries like this?
...a/org/elasticsearch/xpack/inference/queries/SemanticSparseVectorQueryRewriteInterceptor.java
Outdated
Show resolved
Hide resolved
Discussed this offline with the team, and we decided that the use case of not specifying inference ID should happen in this PR. So, I am moving it back to Draft status until it's ready for re-review. |
…re expected to fail for now
…ing search inference IDs from field
…related test failures)
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 👍
// Edge case, where inference_id was not specified in the request, | ||
// but we did not intercept this and rewrite to a query o field with | ||
// pre-configured inference. So we trap here and output a nicer error message. | ||
throw new IllegalArgumentException("inference_id required to perform vector search on query string"); |
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.
Including the field name and the (wrong) field type might help users diagnose the problem
throw new IllegalArgumentException("inference_id required to perform vector search on query string"); | |
throw new IllegalArgumentException("inference_id required to perform a sparse_vector query on sparse_vector field [" + fieldName + "]"); |
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.
Nice refactoring! 👏
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.
I really like the refactoring. I think the change overall is good.
I would REALLY like a QueryInterceptorTestCase
and then two sets of tests
SemanticMatchQueryRewriteInterceptorTests
and SemanticQueryRewriteInterceptorTests
I know all the yaml tests SHOULD be covering every branch, but I think having purpose driven interceptor tests that assert rewritten query types and shapes would be really nice.
- do: | ||
search: | ||
index: test-semantic-text-index | ||
body: | ||
query: | ||
sparse_vector: | ||
field: inference_field | ||
query: "inference test" | ||
|
||
- match: { hits.total.value: 1 } | ||
- match: { hits.hits.0._id: "doc_1" } |
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.
❤️
...src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryRewriteInterceptor.java
Outdated
Show resolved
Hide resolved
@benwtrent As a compromise, I'm adding some Java tests here with the expectation that they'll be cleaned up and fleshed out more in a followup PR. |
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
…lastic#118617) (cherry picked from commit 15bec3c) # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferenceFeatures.java
Adds support for running
sparse_vector
queries directly againstsemantic_text
fields without having to pass in nested queries from the client or knowing the semantic subfields.Example script to test: