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

Update Range mapping type docs #62112

Merged
merged 4 commits into from
Sep 16, 2020
Merged

Conversation

wylieconlon
Copy link

The documentation for this mapping type was missing some important information about how to define and use ranges.

@wylieconlon wylieconlon added the >docs General docs changes label Sep 8, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (>docs)

@elasticmachine elasticmachine added the Team:Docs Meta label for docs team label Sep 8, 2020
@wylieconlon wylieconlon requested a review from a team September 8, 2020 17:56
@jrodewig jrodewig added :Search Foundations/Mapping Index mappings, including merging and defining field types v7.10.0 v7.9.2 v8.0.0 labels Sep 9, 2020
@elasticmachine
Copy link
Collaborator

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

1 similar comment
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Sep 9, 2020
@wylieconlon wylieconlon requested a review from a team September 9, 2020 14:33
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Left a small nit and am not sure if "range" aggs are supported for this range fields, at least I didn't get it to work when I tried, but maybe I'm missing something. My active knowledge is limited here.

@@ -4,6 +4,13 @@
<titleabbrev>Range</titleabbrev>
++++

A field that represents a relation between a lower and upper bound, using the
Copy link
Member

Choose a reason for hiding this comment

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

nit: might be just me, but I feel like the field doesn't represent the "relation" between the bounds but a continuous range of values between those bounds. The relations are then used to define and query the range. Maybe my understanding is off here, not a native speaker, sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I think a couple of high-level examples may make this a bit clearer:

Range field types represent a continuous range of values between an upper and
lower bound. For example, a range field can represent values like any integer
between 1 and 5
or any date in October.

relations `gt`, `gte`, `lt` and `lte`. They can be used for querying, and have
limited support for aggregations. The only supported aggregations are
{ref}/search-aggregations-bucket-histogram-aggregation.html[histogram],
{ref}/search-aggregations-bucket-range-aggregation.html[range] and
Copy link
Member

Choose a reason for hiding this comment

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

Just tried range aggregation on simple integer_range and date_range fields on 7.7 and master, gettting errors for both that these fields are not supported for this type of field. Maybe I'm missing something, what did you check or refer to when adding this?

Copy link
Contributor

@jrodewig jrodewig 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 some feedback around the information hierarchy and some suggestions for improvement.

Let me know if you'd like any help implementing those. Thanks for the PR!

Comment on lines 21 to 22
`date_range`:: A range of date values represented as unsigned 64-bit integer milliseconds elapsed since system epoch.
Date ranges accept the same formats described in the <<date, `date`>> type, but `now` is not allowed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can elaborate a little more for clarity.

Suggested change
`date_range`:: A range of date values represented as unsigned 64-bit integer milliseconds elapsed since system epoch.
Date ranges accept the same formats described in the <<date, `date`>> type, but `now` is not allowed.
`date_range`::
A range of <<date,`date`>> values. Date ranges support various date formats
through the <<mapping-date-format,`format`>> mapping parameter. Regardless of
the format used, date values are parsed into an unsigned 64-bit integer
representing milliseconds since the Unix epoch in UTC. Values containing the
`now` <<date-math,date math>> expression are not supported.

Comment on lines 8 to 11
relations `gt`, `gte`, `lt` and `lte`. They can be used for querying, and have
limited support for aggregations. The only supported aggregations are
{ref}/search-aggregations-bucket-histogram-aggregation.html[histogram],
{ref}/search-aggregations-bucket-range-aggregation.html[range] and
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move the operators and common uses after the range types. I think many users will want to know what range types are available before getting info on how to define or use them.

@@ -4,6 +4,13 @@
<titleabbrev>Range</titleabbrev>
++++

A field that represents a relation between a lower and upper bound, using the
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I think a couple of high-level examples may make this a bit clearer:

Range field types represent a continuous range of values between an upper and
lower bound. For example, a range field can represent values like any integer
between 1 and 5
or any date in October.

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

👍 LGTM. Thanks for the iteration @wylieconlon.

I'll merge and backport when @cbuescher approves.

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, LGTM

@jrodewig jrodewig merged commit 4be761f into elastic:master Sep 16, 2020
jrodewig added a commit that referenced this pull request Sep 16, 2020
jrodewig added a commit that referenced this pull request Sep 16, 2020
@wylieconlon wylieconlon deleted the document-ranges branch December 1, 2020 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs General docs changes :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Docs Meta label for docs team Team:Search Meta label for search team v7.9.2 v7.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants