Skip to content

Rename fields to stored_fields and add docvalue_fields#18992

Merged
jimczi merged 1 commit intoelastic:masterfrom
jimczi:fields_rename
Jun 22, 2016
Merged

Rename fields to stored_fields and add docvalue_fields#18992
jimczi merged 1 commit intoelastic:masterfrom
jimczi:fields_rename

Conversation

@jimczi
Copy link
Copy Markdown
Contributor

@jimczi 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

@jimczi jimczi added >enhancement >breaking :Search/Search Search-related issues that do not fall into other categories v5.0.0-alpha4 labels Jun 21, 2016
@jimczi jimczi added the review label Jun 21, 2016
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see it now. Sorry!

@nik9000
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@nik9000
Copy link
Copy Markdown
Member

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.

`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
Copy link
Copy Markdown
Contributor Author

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 ...

@clintongormley
Copy link
Copy Markdown
Contributor

Re-applied in afe99fc

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

Labels

>breaking :Search/Search Search-related issues that do not fall into other categories v5.0.0-alpha5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants