-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Prevent field caps from failing due to can match failure #134134
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
Prevent field caps from failing due to can match failure #134134
Conversation
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
Note: The first commit is reverting #131111. Before merging I'll split that out of this PR and raise it separately. But it is necessary for the fix to be reviewed here. |
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 work! Approach and tests look good overall, just a few nit comments. The big thing is silently swallowing the exception (which I know we discussed in #116106), but that feels so wrong...
server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesFetcher.java
Show resolved
Hide resolved
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
...nce/src/yamlRestTest/resources/rest-api-spec/test/inference/100_semantic_text_field_caps.yml
Outdated
Show resolved
Hide resolved
...nce/src/yamlRestTest/resources/rest-api-spec/test/inference/100_semantic_text_field_caps.yml
Outdated
Show resolved
Hide resolved
...nce/src/yamlRestTest/resources/rest-api-spec/test/inference/100_semantic_text_field_caps.yml
Outdated
Show resolved
Hide resolved
d9e9315
to
3e5e34d
Compare
run elasticsearch-ci/part-2 |
@Mikep86 I have addressed review comments. This is ready for another look. |
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 good except for two things:
- A missing cluster feature check
- No changelog entry for the bug fix. CI should catch this, but maybe that's broken because we are reverting another changelog entry in this PR. Can you manually add it?
Once those are fixed, there are some logistics about how to merge and backport this:
- Merge #134209 first and merge those changes into this PR, which should cancel out the revert changes here.
- Merge and backport this, which should go smoothly now that the revert changes above are cancelled out.
...e/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_field_mapping.yml
Show resolved
Hide resolved
...c/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_field_mapping_bwc.yml
Show resolved
Hide resolved
@Mikep86 I have added the missing cluster feature checks and the changelog. I'm also fully onboard with the merging plan you laid out. |
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!
`FieldCapabilitiesFetcher` performs a can-match in order to quickly return an empty response if no shard can match. However, if can-match fails for some reason, it can cause the field capabilities request to fail. An example of that is when a semantic query is used as filter. can-match will fail as it won't be able to expand the inference results of the query. In cases like that, it makes no sense to fail the field capabilities request. Instead, we should treat can-match as returning `true` to proceed. This change does that by following suit with other callers of can-match. Fixes elastic#116106
fb31c64
to
933bf28
Compare
@elasticsearchmachine run elasticsearch-ci/part-1 |
@elasticmachine run elasticsearch-ci/part-3 |
) `FieldCapabilitiesFetcher` performs a can-match in order to quickly return an empty response if no shard can match. However, if can-match fails for some reason, it can cause the field capabilities request to fail. An example of that is when a semantic query is used as filter. can-match will fail as it won't be able to expand the inference results of the query. In cases like that, it makes no sense to fail the field capabilities request. Instead, we should treat can-match as returning `true` to proceed. This change does that by following suit with other callers of can-match. Fixes elastic#116106 (cherry picked from commit 7295189) # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferenceFeatures.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java
) `FieldCapabilitiesFetcher` performs a can-match in order to quickly return an empty response if no shard can match. However, if can-match fails for some reason, it can cause the field capabilities request to fail. An example of that is when a semantic query is used as filter. can-match will fail as it won't be able to expand the inference results of the query. In cases like that, it makes no sense to fail the field capabilities request. Instead, we should treat can-match as returning `true` to proceed. This change does that by following suit with other callers of can-match. Fixes elastic#116106 (cherry picked from commit 7295189) # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferenceFeatures.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java # x-pack/plugin/inference/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_field_mapping.yml # x-pack/plugin/inference/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_field_mapping_bwc.yml
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
) `FieldCapabilitiesFetcher` performs a can-match in order to quickly return an empty response if no shard can match. However, if can-match fails for some reason, it can cause the field capabilities request to fail. An example of that is when a semantic query is used as filter. can-match will fail as it won't be able to expand the inference results of the query. In cases like that, it makes no sense to fail the field capabilities request. Instead, we should treat can-match as returning `true` to proceed. This change does that by following suit with other callers of can-match. Fixes elastic#116106 (cherry picked from commit 7295189) # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferenceFeatures.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java # x-pack/plugin/inference/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_field_mapping.yml # x-pack/plugin/inference/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_field_mapping_bwc.yml
…134551) `FieldCapabilitiesFetcher` performs a can-match in order to quickly return an empty response if no shard can match. However, if can-match fails for some reason, it can cause the field capabilities request to fail. An example of that is when a semantic query is used as filter. can-match will fail as it won't be able to expand the inference results of the query. In cases like that, it makes no sense to fail the field capabilities request. Instead, we should treat can-match as returning `true` to proceed. This change does that by following suit with other callers of can-match. Fixes #116106 (cherry picked from commit 7295189)
…) (#134552) `FieldCapabilitiesFetcher` performs a can-match in order to quickly return an empty response if no shard can match. However, if can-match fails for some reason, it can cause the field capabilities request to fail. An example of that is when a semantic query is used as filter. can-match will fail as it won't be able to expand the inference results of the query. In cases like that, it makes no sense to fail the field capabilities request. Instead, we should treat can-match as returning `true` to proceed. This change does that by following suit with other callers of can-match. Fixes #116106 (cherry picked from commit 7295189)
) (#134558) `FieldCapabilitiesFetcher` performs a can-match in order to quickly return an empty response if no shard can match. However, if can-match fails for some reason, it can cause the field capabilities request to fail. An example of that is when a semantic query is used as filter. can-match will fail as it won't be able to expand the inference results of the query. In cases like that, it makes no sense to fail the field capabilities request. Instead, we should treat can-match as returning `true` to proceed. This change does that by following suit with other callers of can-match. Fixes #116106 (cherry picked from commit 7295189)
) (#134557) `FieldCapabilitiesFetcher` performs a can-match in order to quickly return an empty response if no shard can match. However, if can-match fails for some reason, it can cause the field capabilities request to fail. An example of that is when a semantic query is used as filter. can-match will fail as it won't be able to expand the inference results of the query. In cases like that, it makes no sense to fail the field capabilities request. Instead, we should treat can-match as returning `true` to proceed. This change does that by following suit with other callers of can-match. Fixes #116106 (cherry picked from commit 7295189)
…tic#134134) (elastic#134557) `FieldCapabilitiesFetcher` performs a can-match in order to quickly return an empty response if no shard can match. However, if can-match fails for some reason, it can cause the field capabilities request to fail. An example of that is when a semantic query is used as filter. can-match will fail as it won't be able to expand the inference results of the query. In cases like that, it makes no sense to fail the field capabilities request. Instead, we should treat can-match as returning `true` to proceed. This change does that by following suit with other callers of can-match. Fixes elastic#116106 (cherry picked from commit 7295189)
Prevent field_caps from failing when can-match fails
FieldCapabilitiesFetcher
performs a can-match in order to quicklyreturn an empty response if no shard can match. However, if can-match
fails for some reason, it can cause the field capabilities request to fail.
An example of that is when a semantic query is used as filter. can-match
will fail as it won't be able to expand the inference results of the query.
In cases like that, it makes no sense to fail the field capabilities request.
Instead, we should treat can-match as returning
true
to proceed.This change does that by following suit with other callers of can-match.
Fixes #116106