Skip to content

Comments

Allow FetchSubPhaseProcessors to report their required stored fields …#1

Open
MitchLewis930 wants to merge 1 commit intopr_011_beforefrom
pr_011_after
Open

Allow FetchSubPhaseProcessors to report their required stored fields …#1
MitchLewis930 wants to merge 1 commit intopr_011_beforefrom
pr_011_after

Conversation

@MitchLewis930
Copy link

@MitchLewis930 MitchLewis930 commented Jan 30, 2026

PR_011


Note

Medium Risk
Touches core search fetch execution and stored field/source loading decisions, which can affect response contents and performance; changes are broad but largely mechanical with test updates.

Overview
Refactors the search fetch phase so each FetchSubPhaseProcessor reports what it needs via a new storedFieldsSpec() API, and FetchPhase merges these requirements to decide whether to load _source and which stored fields to load.

Introduces a dedicated StoredFieldsPhase (registered in SearchModule) that turns loaded stored fields into hit fields/metadata, updates FetchSubPhase.HitContext to carry the raw loaded fields, and simplifies SearchHit field storage/constructors accordingly; tests/docs are updated and REST/YAML compatibility tests are skipped/adjusted due to the new StoredFieldsPhase showing up in fetch profiling output.

Written by Cursor Bugbot for commit 547c832. This will update automatically on new commits. Configure here.

…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.
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

}

if (fieldType.isStored()) {
storedFields.add(fieldType.name());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable storedFields populated but never read

Low Severity

The storedFields set is created and populated in the loop (adding field names when fieldType.isStored() is true), but it's never actually used. The TODO comment on lines 164-166 acknowledges this, stating the stored fields will be used "in future" but are currently "loaded separately in HighlightUtils." This dead code adds unnecessary computation and can confuse future developers about the intended data flow.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants