Eschew leniency when parsing time zones #77267
Conversation
not-napoleon
added
>bug
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -0,0 +1,55 @@ | |||
| setup: | |||
There was a problem hiding this comment.
This could be in the same file as the rest of the range agg tests, but I'm refactoring that file in a different branch, and wanted to save myself a merge conflict.
| @@ -210,7 +210,9 @@ private Instant parseDateTime(String value, ZoneId timeZone, boolean roundUpIfNo | |||
| return DateFormatters.from(formatter.parse(value)).toInstant(); | |||
| } else { | |||
| TemporalAccessor accessor = formatter.parse(value); | |||
| ZoneId zoneId = TemporalQueries.zone().queryFrom(accessor); | |||
There was a problem hiding this comment.
The docs describe the zone() query as "A lenient query for the ZoneId, falling back to the ZoneOffset", and how do we feel about leniency?
There was a problem hiding this comment.
while this would fix the use case where a mapping expects an offset but the problem will re-highlight itself whe na mapping is using zoneId?
public void testParseOffset() {
// Format string from #76415
DocValueFormat.DateTime parsesZone = new DocValueFormat.DateTime(
DateFormatter.forPattern("uuuu-MM-dd'T'HH:mm:ss.SSSSSSSSSVV"),
ZoneOffset.UTC,
Resolution.MILLISECONDS
);
long expected = 1628719200000L;
ZonedDateTime sample = ZonedDateTime.of(2021, 8, 12, 0, 0, 0, 0, ZoneId.ofOffset("", ZoneOffset.ofHours(2)));
assertEquals("GUARD: wrong initial millis", expected, sample.toEpochSecond() * 1000);
//assertEquals("GUARD: wrong initial string", "2021-08-12T00:00:00.000000000+02:00", parsesZone.format(expected));
long actualMillis = parsesZone.parseLong(
"2021-08-12T00:00:00.000000000CET",
false,
() -> { throw new UnsupportedOperationException("don't use now"); }
);
assertEquals(expected, actualMillis);
}
There was a problem hiding this comment.
actually I just tested this and it would work..
There was a problem hiding this comment.
That's a good test though, we should add it to the suite. Do you mind if I just copy it in?
There was a problem hiding this comment.
I don't mind it at all :) it is a copy of your test with just patterns changed :D
problem is that it is prone to DST changes. ZoneId.ofOffset("", ZoneOffset.ofHours(2))); and 2021-08-12T00:00:00.000000000+02:00 will have to account for this
elasticmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
@elasticmachine run elasticsearch-ci/part-1 |
There was a problem hiding this comment.
I am worried that we introduced a bug in #73955 and we should not set the timezone on parser? but this would require special handling of epoch_seconds as you commented in that PR
I think the fix we have here is best for now
| } | ||
| - match: { hits.total: 1 } | ||
| - length: { aggregations.myagg.buckets: 1 } | ||
| - match: { aggregations.myagg.buckets.0.from: 1628719200000 } |
There was a problem hiding this comment.
I wonder if we could add human readable date in a comment to help with readability?
|
@elasticmachine update branch |
Resolves #76415
In #73955 we made DocValueFormat date formatters always timezone aware. This solved the problem we had with parsing epoch dates, but created a new problem when parsing dates that specified an offset. This solves that problem, in what I hope is the right way. Previously, we used TemporalQueries#zone() which gets the specified zone if available, and falls back to the offset if not. I think we want the opposite behavior though - if we parsed an offset, use that, otherwise use the zone.