Fix inconsistent roundup behavior for date ranges using Temporal object as terms #127739
Conversation
…ct as terms `roundUp` works well for terms that contain date math expressions. Otherwise, if falling through to using roundUp parsers, the behavior can be surprising. Instead, these parsers fill missing date components with the max possible value, e.g. `2099-12` becomes `2099-12-01T23:59:59.999_999_999Z`. Prior to switching to Java time, Joda's LocalTime#toString() consistently formatted time using `HH:mm:ss.SSS`. Without and missing component, roundUp parsers behave just like normal parsers. Java's LocalTime, however, outputs the shortest possible representation and might omit seconds, milliseconds, etc. This interferes badly with the behavior of the roundup parser. And as a result of this, depending on the term, the lower bound could end up larger than the upper bound, e.g. `1970-01-06T09:49Z` becomes `1970-01-06T09:49:59.999Z`. This PR disables roundUp parsers if the lower/upper term is a Temporal object to restore the behavior prior to switching to Java time.
elasticsearchmachine
added
needs:triage
mosche
added
Team:Core/Infra
elasticsearchmachine
added
needs:triage
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
elasticsearchmachine
removed
the
needs:triage
|
Hi @mosche, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Its been a long time since I've been doing any date rounding, so I'd prefer if someone from @elastic/es-core-infra that is currently doing date-math related stuff is taking a look here. I left a small question around maybe avoiding the second "instanceof" in some cases but for correctness would like to defer to another reviewer.
| // `roundUp` may only be used if the term is not a Temporal object. LocalTime#toString() and similar will output | ||
| // the shortest possible representation and might omit seconds, milliseconds, etc. | ||
| // That interferes badly with the behavior of the roundup parser leading to unexpected results. | ||
| boolean mayRoundUp = (term instanceof Temporal) == false; |
There was a problem hiding this comment.
Maybe the second "instanceof" check can be avoided in case we already know that term is a BytesRef from the previous check? Also if "roundUp" is already false, this check can be avoided (i.e. pull it into the second part of the short-circuit && or something similar.
There was a problem hiding this comment.
thx, I've addressed your comment
@rjernst I added two very explicit tests to demonstrate the issue:
|
There was a problem hiding this comment.
Thanks for the tests. I'm not sure about this change. The round up behavior is intentional, or at least by how we define round up at the moment. In the date math parser the flag is called roundUpIfNoTime. I wonder if the issue is there then, that we should not fill in nanos if milliseconds are already provided.
|
|
||
| // explicitly not using nextFrom here to provide a concrete, visual example. | ||
| // see the behavioral difference compared to testRangeQueryIntersectsAdjacentDates | ||
| ZonedDateTime from = ZonedDateTime.parse("2025-05-01T14:10:00.000Z"); |
There was a problem hiding this comment.
This is a little deceptive, the parser used in rangeQuery will call toString on this. I think it would be slightly clearer if the test used Strings instead, which matches what would come from a real query, not a ZonedDateTime object.
I wonder if that is the issue with the original test failures: they're passing in ZonedDateTime objects and subject to their toString behavior, but in production this wouldn't happen, we get strings directly from the request.
There was a problem hiding this comment.
If we don't care about ZonedDateTime objects, there's no problem here and the failing tests have simply tested for the wrong thing... the problem is really only the difference between Joda vs Java time behaving differently for the toString (pls read the PR description for the details)
There was a problem hiding this comment.
Yeah I don't think ZonedDateTime is possible outside tests, see where eg from is set in RangeQueryBuilder, it basically comes down to parsing xcontent, where we only have strings and numbers. The entire point of the date math parser is to turn a string into a ZonedDateTime, so it doesn't make sense that we would try to take an already realized ZonedDateTime, turn it back into a string, and then get a ZonedDateTime.
| ZonedDateTime from = ZonedDateTime.parse("2025-05-01T14:10:00.000Z"); | ||
| ZonedDateTime to = ZonedDateTime.parse("2025-05-01T14:11:00.000Z"); | ||
|
|
||
| // usage of roundUp parsers is wrong if terms are Temporal objects, otherwise it would arbitrarily change the bounds |
There was a problem hiding this comment.
by "Temporal object" don't you mean all dates?
roundUpworks well for terms that contain date math expressions. Otherwise, if falling through to using roundUp parsers, the behavior can be surprising. Instead, these parsers fill missing date components with the max possible value, e.g.2099-12becomes2099-12-01T23:59:59.999_999_999Z.Prior to switching to Java time, Joda's
LocalTime#toString()consistently formatted time usingHH:mm:ss.SSS. Without and missing component, roundUp parsers behave just like normal parsers.Java's
LocalTime, however, outputs the shortest possible representation and might omit seconds, milliseconds, etc.This interferes badly with the behavior of the roundup parser. And as a result of this, depending on the term, the lower bound could end up larger than the upper bound, e.g.
1970-01-06T09:49Zbecomes1970-01-06T09:49:59.999Z.This PR disables roundUp parsers if the lower/upper term is a Temporal object to restore the behavior prior to switching to Java time.
Fixes #86284
Relates to #86508