Skip to content

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

Merged
merged 2 commits into from
Oct 20, 2021

Conversation

jtibshirani
Copy link
Contributor

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.

@jtibshirani jtibshirani added :Search/Search Search-related issues that do not fall into other categories v8.0.0 labels Oct 18, 2021
search:
rest_total_hits_as_int: true
index: test-index
knn_search:
Copy link
Contributor Author

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.

@jtibshirani jtibshirani mentioned this pull request Oct 18, 2021
17 tasks
@jtibshirani jtibshirani marked this pull request as ready for review October 19, 2021 00:35
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Oct 19, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

cool descriptions!!!

Copy link
Contributor Author

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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@romseygeek romseygeek left a 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.

@jtibshirani
Copy link
Contributor Author

Thank you both for reviews.

@jtibshirani jtibshirani merged commit 97c62a7 into elastic:master Oct 20, 2021
@jtibshirani jtibshirani deleted the knn-endpoint branch October 20, 2021 17:41
@jtibshirani jtibshirani added :Search Relevance/Vectors Vector search and removed :Search/Search Search-related issues that do not fall into other categories labels Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants