Skip to content
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

For the fields fetch phase, avoid reloading stored fields. #58196

Merged
merged 3 commits into from
Jun 17, 2020

Conversation

jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Jun 16, 2020

This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.

This improves the performance of fields loading so that it's very close to
source filtering. See #55363 (comment) for more details.

Relates to #55363.

@jtibshirani jtibshirani added :Search/Search Search-related issues that do not fall into other categories >refactoring labels Jun 16, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jun 16, 2020
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
@jtibshirani jtibshirani requested a review from nik9000 June 16, 2020 20:22
@jtibshirani jtibshirani force-pushed the share-stored-fields branch from 47c46f9 to 2c7279d Compare June 16, 2020 20:22
@jtibshirani jtibshirani requested a review from jimczi June 16, 2020 20:23
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

I left a question, lgtm otherwise

@@ -101,7 +101,8 @@ public void execute(SearchContext context) {
if (!context.hasScriptFields() && !context.hasFetchSourceContext()) {
context.fetchSourceContext(new FetchSourceContext(true));
}
fieldsVisitor = new FieldsVisitor(context.sourceRequested());
boolean loadSource = context.sourceRequested() || context.fetchFieldsContext() != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have the same logic in the else below or do we expect users to not use stored_fields in conjunction with the new fields option ?

Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be safest to add it below too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thanks for catching this. I'll push an update.

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

LGTM

keyword: "x"
integer: 42

- do:
Copy link
Member

Choose a reason for hiding this comment

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

I tend to stick refresh on the index request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 this is nicer, especially when there's only one index request.

@jtibshirani
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

@jtibshirani jtibshirani merged this pull request into elastic:field-retrieval Jun 17, 2020
@jtibshirani jtibshirani deleted the share-stored-fields branch June 17, 2020 17:36
jtibshirani added a commit that referenced this pull request Jun 17, 2020
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jun 26, 2020
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jun 30, 2020
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jun 30, 2020
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 8, 2020
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 14, 2020
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 15, 2020
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 18, 2020
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 21, 2020
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 23, 2020
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Jul 27, 2020
…8196)

This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>refactoring :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants