-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Adding support to exclude semantic_text subfields #127664
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
base: main
Are you sure you want to change the base?
Adding support to exclude semantic_text subfields #127664
Conversation
Hi @Samiul-TheSoccerFan, I've created a changelog YAML for you. |
...e/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_field_mapping.yml
Outdated
Show resolved
Hide resolved
- requires: | ||
cluster_features: "gte_v8.16.0" | ||
reason: field_caps support for semantic_text added in 8.16.0 |
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.
Do we need to define a new cluster feature? As per my understanding, these fields are not expected from field_caps
API so excluding these should not have an impact on the API level or discover. We have also covered backward compatibility through other yaml
file.
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 think it would be good to create a test feature for these tests.
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.
Agreed with @Mikep86 's comments in Slack, but good start!
- requires: | ||
cluster_features: "gte_v8.16.0" | ||
reason: field_caps support for semantic_text added in 8.16.0 |
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 think it would be good to create a test feature for these tests.
/** | ||
* Returns true if the field should be excluded from the field capabilities response. | ||
* This is used to exclude fields that are not useful for the user, such as | ||
* offset_source and inference chunk embeddings. | ||
*/ | ||
private static boolean shouldExcludeField(MappedFieldType ft) { | ||
return ft.typeName().equals("offset_source") | ||
|| ((ft instanceof SparseVectorFieldMapper.SparseVectorFieldType | ||
|| ft instanceof DenseVectorFieldMapper.DenseVectorFieldType | ||
|| ft instanceof KeywordFieldMapper.KeywordFieldType) && ft.name().contains(".inference.chunks")); | ||
} |
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.
Reiterating my message offline, this is a brittle solution. We shouldn't be hard-coding field names to exclude from field caps. Instead, I recommend investigating a solution where we add a flag to MappedFieldType
to control if a field is excluded from field caps.
Update the
fieldCaps
API to excludesemantic_text
subfields in both legacy and new formats.Legacy format:
setup:
Query:
Response before update (Skimmed):
Response after update (Skimmed):
new format:
setup:
Query:
Response before update (Skimmed):
Response after update (Skimmed):