-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fix bug where intercepted semantic knn queries did not respect filters #121410
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
Fix bug where intercepted semantic knn queries did not respect filters #121410
Conversation
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
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
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.
Good catch and nice fix!
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, left some questions to gain more knowledge.
public static final NodeFeature SEMANTIC_KNN_VECTOR_QUERY_REWRITE_INTERCEPTION_SUPPORTED = new NodeFeature( | ||
"search.semantic_knn_vector_query_rewrite_interception_supported" | ||
); | ||
public static final NodeFeature SEMANTIC_KNN_FILTER_FIX = new NodeFeature("search.semantic_knn_filter_fix"); |
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.
is this value semantic_knn_filter_fix
defined somewhere, or does the NodeFeature
require a unique string to identify the fix from a specific version?
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's required for testing, so we only test the fix on versions that have the feature. This is the pattern we use for defining node features.
); | ||
} | ||
|
||
copy.addFilterQueries(original.filterQueries()); |
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.
Curious—how do we ensure that the same filter is not added twice? Also, do we handle a different type of filter using copy.addFilterQuery(new TermsQueryBuilder(IndexFieldMapper.NAME, indices))
?
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 is adding the filters that were already added to the request, we do not need to do further analysis as we're translating the query. Adding the filter on index name allows us to filter to only those indices we want to query.
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.
Adding the filter on index name allows us to filter to only those indices we want to query
I missed this key point. Thank you for the clarifications.
Fixes a bug where
knn
queries onsemantic_text
did not respect configured filters.Example query to test: