Index name expression resolver bwc layer for date parsing #58503
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| --- | ||
| "Index java 8 date without timezone": | ||
|
|
||
| - skip: | ||
| version: " - 6.7.99" | ||
| reason: fixed in 6.8.11 | ||
|
|
||
| - do: | ||
| indices.create: | ||
| index: test_index | ||
| body: | ||
| settings: | ||
| number_of_shards: 1 | ||
| mappings: | ||
| doc: | ||
| properties: | ||
| date_field: | ||
| type: date | ||
| format: "8YYYY-ww" | ||
| - do: | ||
| bulk: | ||
| refresh: true | ||
| body: | ||
| - '{"index": {"_index": "test_index", "_type": "doc", "_id": "1"}}' | ||
| - '{"date_field": "2020-32"}' | ||
| - '{"index": {"_index": "test_index", "_type": "doc", "_id": "2"}}' | ||
| - '{"date_field": "2020-33"}' | ||
|
|
||
| - match: { errors: false } | ||
|
|
||
| - do: | ||
| get: | ||
| index: test_index | ||
| type: doc | ||
| id: 2 | ||
|
|
||
| - match: { _index: test_index } | ||
| - match: { _type: doc } | ||
| - match: { _id: "2"} | ||
| - match: { _version: 1} | ||
| - match: { _source: { "date_field": "2020-33" }} | ||
|
|
||
| - do: | ||
| search: | ||
| index: test_index | ||
| body: { "query": { "range": { "date_field": { "gte": "2020-33" } } } } | ||
|
|
||
| - match: {hits.total: 1 } | ||
| - match: {hits.hits.0._index: test_index } | ||
| - match: {hits.hits.0._type: doc } | ||
| - match: {hits.hits.0._source.date_field: "2020-33" } | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,29 +27,26 @@ | |
| import org.elasticsearch.common.Strings; | ||
| import org.elasticsearch.common.collect.ImmutableOpenMap; | ||
| import org.elasticsearch.common.collect.Tuple; | ||
| import org.elasticsearch.common.regex.Regex; | ||
| import org.elasticsearch.common.settings.Settings; | ||
| import org.elasticsearch.common.time.DateFormatter; | ||
| import org.elasticsearch.common.time.DateFormatters; | ||
| import org.elasticsearch.common.time.DateMathParser; | ||
| import org.elasticsearch.common.util.set.Sets; | ||
| import org.elasticsearch.index.Index; | ||
| import org.elasticsearch.index.IndexNotFoundException; | ||
| import org.elasticsearch.index.IndexSettings; | ||
| import org.elasticsearch.indices.IndexClosedException; | ||
| import org.elasticsearch.indices.InvalidIndexNameException; | ||
|
|
||
| import java.time.ZoneId; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Collection; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.Set; | ||
|
|
@@ -64,12 +61,11 @@ public class IndexNameExpressionResolver { | |
| private final DateMathExpressionResolver dateMathExpressionResolver; | ||
|
|
||
| public IndexNameExpressionResolver(Settings settings) { | ||
| expressionResolvers = Arrays.asList( | ||
| dateMathExpressionResolver = new DateMathExpressionResolver(settings), | ||
| new WildcardExpressionResolver() | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Same as {@link #concreteIndexNames(ClusterState, IndicesOptions, String...)}, but the index expressions and options | ||
|
|
@@ -848,22 +844,23 @@ private static List<String> resolveEmptyOrTrivialWildcard(IndicesOptions options | |
|
|
||
| static final class DateMathExpressionResolver implements ExpressionResolver { | ||
|
|
||
| private static final DateFormatter DEFAULT_DATE_FORMATTER = DateFormatters.forPattern("uuuu.MM.dd"); | ||
| private static final String EXPRESSION_LEFT_BOUND = "<"; | ||
| private static final String EXPRESSION_RIGHT_BOUND = ">"; | ||
| private static final char LEFT_BOUND = '{'; | ||
| private static final char RIGHT_BOUND = '}'; | ||
| private static final char ESCAPE_CHAR = '\\'; | ||
| private static final char TIME_ZONE_BOUND = '|'; | ||
|
|
||
| private final ZoneId defaultTimeZone; | ||
| private final String defaultDateFormatterPattern; | ||
| private final DateFormatter defaultDateFormatter; | ||
|
|
||
| DateMathExpressionResolver(Settings settings) { | ||
| String defaultTimeZoneId = settings.get("date_math_expression_resolver.default_time_zone", "UTC"); | ||
| 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 |
||
| this.defaultDateFormatter = DateFormatters.forPattern("uuuu.MM.dd"); | ||
|
There was a problem hiding this comment. Shouldn't this use the pattern in There was a problem hiding this comment. good catch, this should use the |
||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -930,11 +927,10 @@ String resolveExpression(String expression, final Context context) { | |
| int dateTimeFormatLeftBoundIndex = inPlaceHolderString.indexOf(LEFT_BOUND); | ||
| String mathExpression; | ||
| String dateFormatterPattern; | ||
| DateFormatter dateFormatter; | ||
| final ZoneId timeZone; | ||
| if (dateTimeFormatLeftBoundIndex < 0) { | ||
| mathExpression = inPlaceHolderString; | ||
| dateFormatter = defaultDateFormatter; | ||
| timeZone = defaultTimeZone; | ||
| } else { | ||
|
|
@@ -947,23 +943,25 @@ String resolveExpression(String expression, final Context context) { | |
| inPlaceHolderString); | ||
| } | ||
| mathExpression = inPlaceHolderString.substring(0, dateTimeFormatLeftBoundIndex); | ||
| String dateFormatterPatternAndTimeZoneId = | ||
| inPlaceHolderString.substring(dateTimeFormatLeftBoundIndex + 1, inPlaceHolderString.length() - 1); | ||
| int formatPatternTimeZoneSeparatorIndex = dateFormatterPatternAndTimeZoneId.indexOf(TIME_ZONE_BOUND); | ||
| if (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 |
||
| dateFormatterPatternAndTimeZoneId.substring(formatPatternTimeZoneSeparatorIndex + 1)); | ||
| } else { | ||
| dateFormatterPattern = dateFormatterPatternAndTimeZoneId; | ||
| timeZone = defaultTimeZone; | ||
| } | ||
| dateFormatter = DateFormatter.forPattern(dateFormatterPattern); | ||
| } | ||
| DateFormatter formatter = dateFormatter.withZone(timeZone); | ||
| DateMathParser dateMathParser = formatter.toDateMathParser(); | ||
| long millis = dateMathParser.parse(mathExpression, context::getStartTime, false, timeZone); | ||
|
|
||
|
|
||
| String time = formatter.formatMillis(millis); | ||
| beforePlaceHolderSb.append(time); | ||
| inPlaceHolderSb = new StringBuilder(); | ||
|
|
@@ -1007,18 +1005,4 @@ String resolveExpression(String expression, final Context context) { | |
| return beforePlaceHolderSb.toString(); | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ 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