-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Disallow kNN searches on nested vector fields #79403
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
Conversation
search: | ||
rest_total_hits_as_int: true | ||
index: test-index | ||
knn_search: |
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 are extra changes here because I caught and fixed some mistakes in these REST tests.
Pinging @elastic/es-search (Team:Search) |
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.
@jtibshirani Thanks, this LGTM!
@@ -28,6 +42,9 @@ setup: | |||
body: | |||
name: moose.jpg | |||
vector: [-0.5, 100.0, -13, 14.8, -156.0] | |||
comments: | |||
- body: "what a great moose" |
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.
cool descriptions!!!
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.
hehe, I got bored with my usual test examples :)
* `bar.baz`, then calling this method with `foo.bar.baz` will return | ||
* the path `foo`, skipping over the object-but-not-nested `foo.bar` | ||
*/ | ||
public String getNestedParent(String path) { |
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.
it seems that the previous getNestedParent
will return null
if path
is not an objectMapper? But from how this method was used, it looks like this check is not important. So this change LGTM.
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.
Right, this behavior is different but I also couldn't find any place where this check was important.
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! I have a separate issue to clean up all these nested methods but this is good for now.
Thank you both for reviews. |
The new kNN endpoint currently doesn't support searches on nested fields. This
PR updates the validation logic to detect this case and throw a clear error. It
also adds tests for kNN search when there are nested documents.
Relates to #78473.