Make range query rounding consistent

[cbuescher]

Currently the rounding for date math used in range queries behaves differently
for date and date_range as explained in #50009. The behaviour on date
fields is the one we document in
https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-range-query.html#range-query-date-math-rounding.
This change adapts the rounding behaviour for RangeType.DATE so it uses the same
logic as the date type.

Closes #50009

[elasticmachine]

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

[cbuescher]

Opening as WIP to illustrate the issue better. Original issue needs to be discussed to decide whether we see the current behaviour as a bug, in which case this could be considered a bugfix, or as the standing behaviour, in which case this should be documented as a breaking issue if we want to change it to make the date/date_range behaviour consistent.

[cbuescher]

@elasticmachine update branch

[cbuescher]

@jpountz thanks for the review, after you took a look I still extended testing a bit by adding some randomized testing to make sure we treat bounds the same on date and date_range fields with this change now. I also added a note to the breaking search changes. I'm not sure if I should go into more details on that note or if there is a better place for that, just in case here's another attempt at summarizing this change and what to do in case users want to keep the "old" behaviour:

---

Before this change, a range query with an upper bound not specified up to milliseconds like e.g. ”lte”:”2019-12-09” or when using date math day rounding like ”lte”:”2019-12-09||/d” would translate to an included upper bound of 2019-12-09T00:00:00.000.
With this change, the above will be rounded up to include the full day up to 2019-12-09T23:59:59.999

The reverse will happen for lower bounds specified with “gt”. Both ”gt”:”2019-12-09” and ”gt”:”2019-12-09||/d” are currently translated to 2019-12-09T00:00:00. With this change we will translate to start of next day 2019-12-10T00:00:00
Both of these cases are already handled like this on a plain date field.

In order for user keep the current behaviour, they will need to change their range queries on date_range field using lte or gt from e.g.

"lte”:”2019-12-09" -> ”lt”:”2019-12-09” (or fully specify ”lte”:”2019-12-09T00:00:00.000)
"gt”:”2019-12-09" -> ”gte”:”2019-12-09” (or fully specify ”gt”:”2019-12-09T00:00:00.000)

---

Can you take another look at the test changes and if you think the phrasing in the migration docs is sufficient?
I remember we talked about the possibility of backporting this to 7.x but currently I'd refrain from that because the behavioural changes of the query are subtle and easy to miss. Let me know if you know of any other good other place to document this.

[jpountz]

The change to src/main look good, but I'd like to avoid relying on string representations in tests. Could we instead test a rounded range against the expected non-rounded range, e.g. something like assertEquals(range on "gt 2020-01-20", range on "gt 2020-01-20 23:59:59.999Z")?

Since this is a bug fix, I think this should get backported to 7.x.

[jpountz]

Rephrase this title, and maybe the paragraph below as well to make it clear that only date ranges changed behavior? A majority of users are using date fields but only a minority uses date_range fields, so it would help if users could notice that this change doesn't apply to them only by looking at the title.

[cbuescher]

@jpountz thanks for the review, I pushed updates to the migration notes and found a way of avoiding relying on string representations in the tests comparing the bounds with the behaviour on the plain date field. Do you want to take another look at those changes or do you think this is good to go?

[jpountz]

Two small comments, LGTM otherwise. Thanks for fixing it @cbuescher.

[jpountz]

this parameter is supposed to only exist for the 6.x -> 7.x transition, let's use track_total_hits: true instead?

[cbuescher]

Just saw that “rest_total_hits_as_int” is still used all over the place in the yml tests, also in other tests in same test suite. I agree it would be good to get rid of it when its supposed to go away, but I’d like to keep it for consistency with the rest atm. Maybe we should open an issue to clean up all of those in a separate PR?