Allow FetchSubPhaseProcessors to report their required stored fields …#1
Open
MitchLewis930 wants to merge 1 commit intopr_011_beforefrom
Open
Allow FetchSubPhaseProcessors to report their required stored fields …#1MitchLewis930 wants to merge 1 commit intopr_011_beforefrom
MitchLewis930 wants to merge 1 commit intopr_011_beforefrom
Conversation
…lastic#91269) Loading of stored fields is currently handled directly in FetchPhase, with some fairly complex logic examining various bits of the FetchContext to work out what fields need to be loaded. This is further complicated by synthetic source, which may have its own stored field requirements. This commit tries to separate out these concerns a little by adding a new StoredFieldsSpec record that holds information about which stored fields need to be loaded. Each FetchSubPhaseProcessor can now report a StoredFieldsSpec detailing what its requirements are, and these specs can be merged together, along with requirements from a SourceLoader, to determine up-front what fields should be loaded by the StoredFieldLoader. The stored fields themselves are added into the SearchHit by a new StoredFieldsPhase, which handles alias resolution and value post- processing. The logic to determine when source should be loaded and when not, based on the presence of script fields or stored fields, is moved into FetchContext, which highlights some inconsistencies that can be fixed in follow-up commits.
There was a problem hiding this comment.
Pull request overview
This PR refactors how FetchSubPhaseProcessors report their required stored fields and source dependencies, enabling more efficient field loading during search fetch operations.
Changes:
- Introduced
StoredFieldsSpecto consolidate stored field requirements across fetch sub-phases - Refactored
SearchHitconstruction to use builder pattern for document/metadata fields - Added
StoredFieldsPhaseas a dedicated sub-phase for processing stored fields
Reviewed changes
Copilot reviewed 84 out of 84 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/org/elasticsearch/search/fetch/StoredFieldsSpec.java | New record class defining stored field requirements |
| server/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseProcessor.java | Added storedFieldsSpec() method to interface |
| server/src/main/java/org/elasticsearch/search/SearchHit.java | Refactored constructor and field handling to use builder pattern |
| server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java | Major refactor to consolidate stored field loading logic |
| server/src/main/java/org/elasticsearch/search/fetch/subphase/StoredFieldsPhase.java | New dedicated phase for processing stored fields |
| Multiple test files | Updated SearchHit instantiation to use simplified constructors |
| server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java | Changed field type visibility from INSTANCE to FIELD_TYPE |
| server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java | Changed field type visibility from INSTANCE to FIELD_TYPE |
| server/src/main/java/org/elasticsearch/index/mapper/LegacyTypeFieldMapper.java | Changed field type visibility and added FIELD_TYPE constant |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Override | ||
| public boolean hasScriptFields() { | ||
| return scriptFields != null; | ||
| return scriptFields != null && scriptFields.fields().isEmpty() == false; |
There was a problem hiding this comment.
The double negation pattern isEmpty() == false reduces readability. Use !scriptFields.fields().isEmpty() for clearer intent.
Suggested change
| return scriptFields != null && scriptFields.fields().isEmpty() == false; | |
| return scriptFields != null && !scriptFields.fields().isEmpty(); |
| @Override | ||
| public boolean hasScriptFields() { | ||
| return scriptFields != null; | ||
| return scriptFields != null && scriptFields.fields().isEmpty() == false; |
There was a problem hiding this comment.
The double negation pattern isEmpty() == false reduces readability. Use !scriptFields.fields().isEmpty() for clearer intent.
Suggested change
| return scriptFields != null && scriptFields.fields().isEmpty() == false; | |
| return scriptFields != null && !scriptFields.fields().isEmpty(); |
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
2 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.
PR_011