-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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 _size and _doc_count to fields output #70575
Conversation
Currently metadata fields like `_size` or `_doc_count` cannot be retrieved using the fields API. With this change, we allow this if the field is explicitely queried for using its name, but won't include metadata fields when e.g. requesting all fields via "*". With this change, not all metadata fields will be retrievable by using its name, but support for "_size" and "_doc_count" (which is fetched from source) is added. Support for other metadata field types will need to be decided case by case and an appropriate ValueFetcher needs to be supplied. Relates to elastic#63569
Pinging @elastic/es-search (Team:Search) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! It'd be nice to update mapper-size.asciidoc
with this simplified approach, maybe removing references to script_fields
and docvalue_fields
.
server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java
Outdated
Show resolved
Hide resolved
@jtibshirani thanks, I added the suggested small changes. I left the script example in the docs since I find it useful to also mention how the field can be accessed in scripts, but I replaced the doc values retrieval example with one using the fields API. |
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed we merged the new _tier
metadata field. Do we plan to handle that in this PR, or a follow-up?
docs/plugins/mapper-size.asciidoc
Outdated
@@ -85,9 +81,4 @@ GET my-index-000001/_search | |||
<4> Uses a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to remove this reference to script_fields
too? It doesn't seem so useful, and I think we'll be phasing out script_fields
now that we have runtime fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it here to show an example of general access in scripts, the script_field
seems to provide a simple way of giving that example. But I can remove it if you don't find it useful. I think I do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong preference from me. As a compromise we could just swap the order to emphasize fields
is the preferred way to retrieve its value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea.
I saw the issue, wasn't sure it was merged already. And yes, if we want to support that meta data field I'd prefer this being a follow up, seeing this PR is close to merging I prefer not to include it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple last documentation suggestions, once those are addressed it looks ready to merge!
docs/plugins/mapper-size.asciidoc
Outdated
@@ -85,9 +81,4 @@ GET my-index-000001/_search | |||
<4> Uses a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong preference from me. As a compromise we could just swap the order to emphasize fields
is the preferred way to retrieve its value?
@@ -26,7 +26,7 @@ PUT my-index-000001 | |||
-------------------------- | |||
|
|||
The value of the `_size` field is accessible in queries, aggregations, scripts, | |||
and when sorting: | |||
and when sorting. It can be retrieved using the {ref}/search-fields.html#search-fields-param[fields API]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed this: we've been trying not to call it the 'fields API' in official docs. It has caused confusion in the past because some users thought it was a separate API endpoint. We could say fields
parameter or fields
option instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, not sure this was updated before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I missed that one, only changed the mention below in the example. Will push a small online-fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected this.
Currently metadata fields like `_size` or `_doc_count` cannot be retrieved using the fields API. With this change, we allow this if the field is explicitely queried for using its name, but won't include metadata fields when e.g. requesting all fields via "*". With this change, not all metadata fields will be retrievable by using its name, but support for "_size" and "_doc_count" (which is fetched from source) is added. Support for other metadata field types will need to be decided case by case and an appropriate ValueFetcher needs to be supplied. Relates to #63569
@jtibshirani given the GA discussion, do you think this should also go into 7.12.x? |
I don't think it needs to go into 7.12, since it is an enhancement and doesn't change the API. We made sure to list it under 'other changes' on the meta issue (#60985) to clarify it doesn't affect the API. |
Currently metadata fields like
_size
or_doc_count
cannot be retrieved usingthe fields API. With this change, we allow this if the field is explicitely
queried for using its name, but won't include metadata fields when e.g.
requesting all fields via "*".
With this change, not all metadata fields will be retrievable by using its name,
but support for "_size" and "_doc_count" (which is fetched from source) is
added. Support for other metadata field types will need to be decided case by
case and an appropriate ValueFetcher needs to be supplied.
Relates to #63569