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

Rename `fields` to `stored_fields` and add `docvalue_fields` #18992

Merged
merged 1 commit into from Jun 22, 2016

Conversation

Projects
None yet
3 participants
@jimczi
Member

jimczi commented Jun 21, 2016

stored_fields parameter will no longer try to retrieve fields from the _source but will only return stored fields.
fields will throw an exception if the user uses it.
Add docvalue_fields as an adjunct to fielddata_fields which is deprecated. docvalue_fields will try to load the value from the docvalue and fallback to fielddata cache if docvalues are not enabled on that field.

Closes #18943

SearchSourceBuilder.STORED_FIELDS_FIELD + "] to retrieve stored fields or _source filtering " +
"if the field is not stored");
}, SearchSourceBuilder.FIELDS_FIELD, ObjectParser.ValueType.STRING_ARRAY);
PARSER.declareStringArray(InnerHitBuilder::setDocValueFields, SearchSourceBuilder.DOCVALUE_FIELDS_FIELD);

This comment has been minimized.

@nik9000

nik9000 Jun 21, 2016

Contributor

The commit message says fielddata_fields is deprecated but you've removed it above. Maybe it should be a deprecated alias for docvalue_fields?

@nik9000

nik9000 Jun 21, 2016

Contributor

The commit message says fielddata_fields is deprecated but you've removed it above. Maybe it should be a deprecated alias for docvalue_fields?

This comment has been minimized.

@jimczi

jimczi Jun 21, 2016

Member

It's already a deprecated alias for docvalue_fields, see SearchSourceBuilder.DOCVALUE_FIELDS_FIELD declaration.

@jimczi

jimczi Jun 21, 2016

Member

It's already a deprecated alias for docvalue_fields, see SearchSourceBuilder.DOCVALUE_FIELDS_FIELD declaration.

This comment has been minimized.

@nik9000

nik9000 Jun 21, 2016

Contributor

I see it now. Sorry!

@nik9000

nik9000 Jun 21, 2016

Contributor

I see it now. Sorry!

@nik9000

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java
@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Jun 21, 2016

Contributor

I wonder if we're ok removing the methods that you've deprecated rather than just deprecating them? If you do keep them as deprecated can you add a note about when we expect to remove them? Notes like that are nice because they make it obvious to the caller and to us when we can remove them.

Contributor

nik9000 commented Jun 21, 2016

I wonder if we're ok removing the methods that you've deprecated rather than just deprecating them? If you do keep them as deprecated can you add a note about when we expect to remove them? Notes like that are nice because they make it obvious to the caller and to us when we can remove them.

if (sField != null) {
if (!Strings.hasText(sField)) {
searchSourceBuilder.noFields();
searchSourceBuilder.noStoredFields();
} else {
String[] sFields = Strings.splitStringByCommaToArray(sField);
if (sFields != null) {

This comment has been minimized.

@nik9000

nik9000 Jun 21, 2016

Contributor

I think you can drop the null check. It returns an empty array instead.

@nik9000

nik9000 Jun 21, 2016

Contributor

I think you can drop the null check. It returns an empty array instead.

@nik9000

View changes

Show outdated Hide outdated .../src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java
@nik9000

View changes

Show outdated Hide outdated .../src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java
@nik9000

View changes

Show outdated Hide outdated docs/reference/migration/migrate_5_0/search.asciidoc
[source,js]
--------------------------------------------------
GET /_search

This comment has been minimized.

@nik9000

nik9000 Jun 21, 2016

Contributor

It'd be awesome to have a complete example here that indexes a document, does this search, and then shows what the output looks like. I don't think most users understand what they are getting with this option.

@nik9000

nik9000 Jun 21, 2016

Contributor

It'd be awesome to have a complete example here that indexes a document, does this search, and then shows what the output looks like. I don't think most users understand what they are getting with this option.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Jun 21, 2016

Contributor

LGTM. I left only minor stuff. If you want I can make the documentation modifications that I proposed after you merge.

Contributor

nik9000 commented Jun 21, 2016

LGTM. I left only minor stuff. If you want I can make the documentation modifications that I proposed after you merge.

Rename `fields` to `stored_fields` and add `docvalue_fields`
`stored_fields` parameter will no longer try to retrieve fields from the _source but will only return stored fields.
`fields` will throw an exception if the user uses it.
Add `docvalue_fields` as an adjunct to `fielddata_fields` which is deprecated. `docvalue_fields` will try to load the value from the docvalue and fallback to fielddata cache if docvalues are not enabled on that field.

Closes #18943
@jimczi

This comment has been minimized.

Show comment
Hide comment
@jimczi

jimczi Jun 22, 2016

Member

Thanks @nik9000 !

It'd be awesome to have a complete example here that indexes a document, does this search, and then shows what the output looks like. I don't think most users understand what they are getting with this option.

I think we should first remove the support for the fielddata cache. It should not be recommended and IMO it would be great if we can remove it completely. I'll open another issue to discuss ...

Member

jimczi commented Jun 22, 2016

Thanks @nik9000 !

It'd be awesome to have a complete example here that indexes a document, does this search, and then shows what the output looks like. I don't think most users understand what they are getting with this option.

I think we should first remove the support for the fielddata cache. It should not be recommended and IMO it would be great if we can remove it completely. I'll open another issue to discuss ...

@jimczi jimczi removed the review label Jun 22, 2016

@jimczi jimczi merged commit a379d62 into elastic:master Jun 22, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@jimczi jimczi deleted the jimczi:fields_rename branch Jun 22, 2016

@clintongormley

This comment has been minimized.

Show comment
Hide comment
@clintongormley

clintongormley Jul 4, 2016

Member

Re-applied in afe99fc

Member

clintongormley commented Jul 4, 2016

Re-applied in afe99fc

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