-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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 sparse_vector query #108254
Add sparse_vector query #108254
Conversation
x-pack/plugin/core/build.gradle
Outdated
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.
Auto formatting by IntelliJ
…ted the teardown. :sweat-smile:
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, thank you for the iterations!
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, good work!
Some doc changes I think are important - can be done on a separate PR but let's not forget about them ;)
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.
We should modify the sparse vector field type doc to specify that sparse_vector
is the preferred query to use instead of text_expansion
.
We could say that text_expansion still works but will be deprecated in the future or something along those lines
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 have a followup issue to deprecate the text_expansion
query as well.
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.
There is a lot of other documentation that should be updated (examples, etc.) but those will be updated in a followup dedicated PR
...in/core/src/main/java/org/elasticsearch/xpack/core/ml/search/WeightedTokensQueryBuilder.java
Outdated
Show resolved
Hide resolved
...in/core/src/main/java/org/elasticsearch/xpack/core/ml/search/WeightedTokensQueryBuilder.java
Show resolved
Hide resolved
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/queries/SparseVectorQueryBuilder.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/queries/SparseVectorQueryBuilder.java
Outdated
Show resolved
Hide resolved
...tests-with-security/src/test/resources/rest-api-spec/test/multi_cluster/50_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.
@kderusso Excellent job on main code, documentation and tests! I left some clarifying questions, but nothing major.
@elasticmachine update branch |
Relates to #106261
This PR introduces a new
sparse_vector
query that combines the functionality of thetext_expansion
andweighted_tokens
queries. Eventually this query will replace the other two queries.Actions that will occur in future PRs:
text_expansion
andweighted_tokens
queriesweighted_tokens
query in theSparseVectorQueryBuilder
sparse_vector
query outside of the ML plugin (requires some inference API work)Examples of how to use this new query type: