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

Add the ability to disable the retrieval of the stored fields entirely #20026

Merged
merged 1 commit into from
Aug 24, 2016
Merged

Add the ability to disable the retrieval of the stored fields entirely #20026

merged 1 commit into from
Aug 24, 2016

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Aug 17, 2016

This is a spin off of #19918 (comment)

This change adds a special field named none that allows to disable the retrieval of the stored fields in a search request or in a TopHitsAggregation.

To completely disable stored fields retrieval (including disabling metadata fields retrieval such as _id or _type) use none:

POST _search
{
   "stored_fields": "_none_"
}

@jimczi jimczi added >feature discuss :Search/Search Search-related issues that do not fall into other categories v5.0.0-beta1 labels Aug 17, 2016
@clintongormley
Copy link
Contributor

i like the looks of this. One question: what if I want (eg) _id and _index, but not _type? Could I specify: stored_fields: [ _none_, _id, _index ]?

Or maybe this doesn't matter as the real cost is retrieving stored fields, rather than serialising them (i think?)

@jimczi
Copy link
Contributor Author

jimczi commented Aug 18, 2016

Or maybe this doesn't matter as the real cost is retrieving stored fields, rather than serialising them (i think?)

Yes that's the spirit. Don't think we should allow to enable/disable metadata field separately.
_none_ is not about metadata fields, it just ensures that nothing is retrieved from the stored_fields, this is why it cannot be put inside an array.

--------------------------------------------------
GET /_search
{
"stored_field": "_none_",
Copy link
Contributor

Choose a reason for hiding this comment

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

should be stored_fields (plural)

@jpountz
Copy link
Contributor

jpountz commented Aug 19, 2016

+1 to forbid loading anything from stored fields when _none_ is used.

@jpountz
Copy link
Contributor

jpountz commented Aug 19, 2016

I am a bit on the fence about making the REST and Java APIs diverge (REST only has stored_fields while Java has setStoredFields, setNoStoredFields and setDisableStoredFields). Could we keep it a string list in the Java API as well and disable stored fields when the list has a single entry which is _none_?

@jimczi
Copy link
Contributor Author

jimczi commented Aug 22, 2016

Thanks @jpountz. I pushed another commit to address your comments.

@jpountz
Copy link
Contributor

jpountz commented Aug 23, 2016

@jimferenczi It seems that GetRequest still has a different API?

@jimczi
Copy link
Contributor Author

jimczi commented Aug 23, 2016

It seems that GetRequest still has a different API?

Yes sorry about that, this is a leftover. This PR should not touch the GetRequest at all.
I pushed another commit to cleanup. I made modifications in InnerHitBuilder, TopHitsAggregationBuilder and SearchRequestBuilder and the GetRequest remains unchanged.
I've also move some logics in the parsing to share more codes and added rest tests.
@jpountz can you take another look ?

* Sets the fields to load and return as part of the search request. If none
* are specified, the source of the document will be returned.
* Adds stored fields to load and return (note, it must be stored) as part of the search request.
* To disable the stored fields entirely (source and metadata fields) use {@code storedField("_none_")}.
Copy link
Contributor

@jpountz jpountz Aug 24, 2016

Choose a reason for hiding this comment

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

s/storedField("_none_")/storedFields("_none_")/ ?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh nevermind I see there is a storedField method too

@jpountz
Copy link
Contributor

jpountz commented Aug 24, 2016

I left some comments.

@jimczi
Copy link
Contributor Author

jimczi commented Aug 24, 2016

Thanks @jpountz , I pushed a commit to address your comments.

}

private StoredFieldsContext(List<String> fieldNames) {
Objects.requireNonNull("fieldNames must not be null");
Copy link
Contributor

Choose a reason for hiding this comment

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

you should pass fieldNames as an argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oups :(

@jpountz
Copy link
Contributor

jpountz commented Aug 24, 2016

LGTM

This change adds a special field named _none_ that allows to disable the retrieval of the stored fields in a search request or in a TopHitsAggregation.

To completely disable stored fields retrieval (including disabling metadata fields retrieval such as _id or _type) use _none_ like this:

````
POST _search
{
   "stored_fields": "_none_"
}
````
@jimczi jimczi merged commit 50b47aa into elastic:master Aug 24, 2016
@jimczi
Copy link
Contributor Author

jimczi commented Aug 24, 2016

Thanks @jpountz !

@jimczi jimczi deleted the disable_stored_fields branch February 3, 2017 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss >feature :Search/Search Search-related issues that do not fall into other categories v5.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants