Don't serialize endpoint metadata in semantic text mappings#145021
Merged
Mikep86 merged 12 commits intoelastic:mainfrom Mar 30, 2026
Merged
Don't serialize endpoint metadata in semantic text mappings#145021Mikep86 merged 12 commits intoelastic:mainfrom
Mikep86 merged 12 commits intoelastic:mainfrom
Conversation
…nerating XContent
Collaborator
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
Collaborator
|
Hi @Mikep86, I've created a changelog YAML for you. |
DonalEvans
reviewed
Mar 26, 2026
...ugin/inference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextField.java
Show resolved
Hide resolved
jonathan-buttner
approved these changes
Mar 26, 2026
| @Override | ||
| public ToXContentObject getFilteredXContentObject() { | ||
| return this::toXContent; | ||
| return (b, p) -> toXContent(b, p, false); |
Contributor
There was a problem hiding this comment.
Ideally in the future we can remove MinimalServiceSettings from implementing ServiceSettings. Maybe when we get around to doing that we can rename the method then. The other option we have is to leverage the Params instead which I think is the typical way to handle this.
...ugin/inference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextField.java
Show resolved
Hide resolved
| @@ -329,7 +329,11 @@ public Builder( | |||
| () -> null, | |||
| (n, c, o) -> SemanticTextField.parseModelSettingsFromMap(o), | |||
| mapper -> ((SemanticTextFieldType) mapper.fieldType()).modelSettings, | |||
| XContentBuilder::field, | |||
| (b, n, v) -> { | |||
Contributor
There was a problem hiding this comment.
If we wanted to use Params here instead, I think we'd do something like this:
(b, n, v) -> {
if (v != null) {
b.field(MODEL_SETTINGS_FIELD, v, new ToXContent.MapParams(Map.of("exclude_metadata", "true")));
}
}
Contributor
Author
|
@elasticmachine update branch |
mouhc1ne
pushed a commit
to shmuelhanoch/elasticsearch
that referenced
this pull request
Mar 31, 2026
ncordon
pushed a commit
to ncordon/elasticsearch
that referenced
this pull request
Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #141393,
MinimalServiceSettingswas updated to add endpoint metadata. This change introduced a bug (see #144969) where endpoint metadata is serialized insemantic_textindex mappings. Endpoint metadata is only necessary when selecting the default inference service to use; once the inference service is selected it is no longer useful and can be discarded.This PR fixes that by implementing filtered XContent for
MinimalServiceSettingsthat excludes endpoint metadata. An alternative approach considered was to make a new class,MappingServiceSettings, that models all params inMinimalServiceSettingsexcept for endpoint metadata. We did not choose to go with that for a couple reasons:semantic_textindex mappings on disk with endpoint metadata. We need to know how to parse those or the mappings will be invalid, breaking Elasticsearch. SeetestMappingWithEndpointMetadatafor a test validating this case with the chosen approach.semantic_textmodel settings beingMinimalServiceSettings, such as when updating the inference ID. Using a new class likeMappingServiceSettingsbreaks this logic, and would require more significant changes to resolve.Nothing precludes us from refactoring at a later time to introduce a new class like
MappingServiceSettings. This PR aims to fix the bug with the fewest and simplest changes possible so that it can be resolved quickly and without side-effects.Fixes #144969