Index name expression resolver bwc layer for date parsing #58503
Conversation
…lastic#50791)"" This reverts commit 662d3ac
|
Pinging @elastic/es-core-infra (:Core/Infra/Core) |
| expressionResolvers = Arrays.asList( | ||
| dateMathExpressionResolver, | ||
| new WildcardExpressionResolver()); | ||
| dateMathExpressionResolver = new DateMathExpressionResolver(settings), |
There was a problem hiding this comment.
nit: why is this moved here? i think setting a member variable shoudl be done on it's own line, like it was before, not hidden inside a list initialization
There was a problem hiding this comment.
++ agree. It was moved because I just cherrypicked the changes from https://github.com/elastic/elasticsearch/pull/34507/files and there was no conflict on this one
will fix
| this.defaultDateFormatter = DateTimeFormat.forPattern(defaultDateFormatterPattern); | ||
| this.defaultTimeZone = ZoneId.of(defaultTimeZoneId); | ||
| defaultDateFormatterPattern = settings.get("date_math_expression_resolver.default_date_format", "uuuu.MM.dd"); | ||
| this.defaultDateFormatter = DateFormatters.forPattern("uuuu.MM.dd"); |
There was a problem hiding this comment.
Shouldn't this use the pattern in defaultDateFormatterPattern?
There was a problem hiding this comment.
good catch, this should use the defaultDateFormatterPattern
| defaultDateFormatterPattern = settings.get("date_math_expression_resolver.default_date_format", "YYYY.MM.dd"); | ||
| this.defaultDateFormatter = DateTimeFormat.forPattern(defaultDateFormatterPattern); | ||
| this.defaultTimeZone = ZoneId.of(defaultTimeZoneId); | ||
| defaultDateFormatterPattern = settings.get("date_math_expression_resolver.default_date_format", "uuuu.MM.dd"); |
There was a problem hiding this comment.
Since this is in 6.8, shouldn't this default format be prefixed with '8' since we still use joda by default here?
There was a problem hiding this comment.
yes, should be with 8. will fix
| timeZone = DateTimeZone.forID(patternAndTZid.substring(formatPatternTimeZoneSeparatorIndex + 1)); | ||
| dateFormatterPattern | ||
| = dateFormatterPatternAndTimeZoneId.substring(0, formatPatternTimeZoneSeparatorIndex); | ||
| timeZone = ZoneId.of( |
There was a problem hiding this comment.
We previously added the '8' prefix in date formats so that both joda and java formats could be distinguished. I have a vague memory that the zone identifiers do not completely match up between joda and java. Is this correct? If so, what happens if the format specified by the user is one not supported by java?
There was a problem hiding this comment.
we test against this discrepancies in https://github.com/elastic/elasticsearch/blob/6.8/server/src/test/java/org/elasticsearch/common/time/DateUtilsTests.java#L38
as long as timezone dbs are the same in both joda and java (and we make sure about this with this test) then there should always be a match
backports #34507
closes #58481
also closes general parsing issue (no date processors) #58602