Improve mutability handling of Source#source()'s map #141534
Merged
+411
−28
Conversation
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
elasticsearchmachine
added
v9.4.0
external-contributor
tballison
commented
Jan 29, 2026
Contributor
Author
There was a problem hiding this comment.
Wasn't sure if I wanted to create a new class/spin up a new cluster for this one simple test. If there's a better home for the test, let me know.
tballison
commented
Jan 29, 2026
server/src/main/java/org/elasticsearch/index/get/ShardGetService.java
Outdated
Show resolved
Hide resolved
tballison
changed the title
tballison
added
Team:Search Relevance
Collaborator
|
Hi @tballison, I've created a changelog YAML for you. |
Mikep86
reviewed
Jan 30, 2026
Contributor
Mikep86
left a comment
Mikep86
left a comment
There was a problem hiding this comment.
Nice start! A few things to close this out:
- Can you add a similar YAML test to
30_semantic_text_inference_bwc.yml? This is the file for testing against the oldsemantic_textformat. In this case, setting_source_includes=_inference_fieldsshould have no effect. We should check it doesn't cause an error though, because that was a bug we fixed previously (see this test) - We should investigate ways to test the changes to
SearchBasedChangesSnapshot. - Can you update the PR title and changelog to be more specific? This bug fixes retrieving
_inference_fieldsvia_source_includes. There are other ways of getting_inference_fieldsthat work fine.
server/src/main/java/org/elasticsearch/index/get/ShardGetService.java
Outdated
Show resolved
Hide resolved
Comment on lines
234
to
236
| var newSource = new HashMap<>(originalSource.source()); | ||
| newSource.put(InferenceMetadataFieldsMapper.NAME, values.get(0)); | ||
| return Source.fromMap(newSource, originalSource.sourceContentType()); |
Contributor
There was a problem hiding this comment.
Nice catch here! I wonder how we test this change?
Contributor
There was a problem hiding this comment.
I investigated how to test this and the short version is this path is already tested by SemanticInferenceMetadataFieldsRecoveryTests. I think we got lucky that the test works because originalSource just happens to store source in a mutable map. But we can't count on that always being the case, so we should keep this change.
@jimczi WDYT?
...rence/src/yamlRestTest/resources/rest-api-spec/test/inference/30_semantic_text_inference.yml
Outdated
Show resolved
Hide resolved
...rence/src/yamlRestTest/resources/rest-api-spec/test/inference/30_semantic_text_inference.yml
Outdated
Show resolved
Hide resolved
...rence/src/yamlRestTest/resources/rest-api-spec/test/inference/30_semantic_text_inference.yml
Outdated
Show resolved
Hide resolved
tballison
changed the title
Contributor
Author
|
@elasticmachine update branch |
…s from semantic_text field
…archBasedChangesSnapshot
…ec/test/inference/30_semantic_text_inference.yml Co-authored-by: Mike Pellegrini <[email protected]>
tballison
force-pushed
the
issues/141075
branch
from
91a3268 to
6195720
Compare
# Conflicts: # server/src/main/java/org/elasticsearch/index/IndexFeatures.java
elasticsearchmachine
added
the
Team:Search Relevance
Collaborator
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
tballison
changed the title
jimczi
reviewed
Feb 3, 2026
server/src/main/java/org/elasticsearch/search/lookup/Source.java
Outdated
Show resolved
Hide resolved
benwtrent
reviewed
Feb 3, 2026
…to avoid unnecessary copying
benwtrent
approved these changes
Feb 3, 2026
jimczi
approved these changes
Feb 3, 2026
jfreden
pushed a commit
to jfreden/elasticsearch
that referenced
this pull request
Feb 4, 2026
Co-authored-by: Mike Pellegrini <[email protected]>
spinscale
pushed a commit
to spinscale/elasticsearch
that referenced
this pull request
Feb 4, 2026
Co-authored-by: Mike Pellegrini <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Wrap source for mutability to allow updating with _inference_fields.
Fixes #141075
Is related to: #136545