From 8608f593ff72d149c6f91036ed478321df0d611b Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 16 Jan 2024 16:25:11 +0100 Subject: [PATCH 1/5] Fix routing_path when template has multiple path_match and multi-fields --- .../DataStreamIndexSettingsProvider.java | 4 +- .../DataStreamIndexSettingsProviderTests.java | 49 +++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java b/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java index 519499addd77e..090c2ee714eed 100644 --- a/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java +++ b/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java @@ -31,7 +31,6 @@ import java.io.UncheckedIOException; import java.time.Instant; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Locale; @@ -182,7 +181,8 @@ private List findRoutingPaths(String indexName, Settings allSettings, Li // Since FieldMapper.parse modifies the Map passed in (removing entries for "type"), that means // that only the first pathMatch passed in gets recognized as a time_series_dimension. To counteract // that, we wrap the mappingSnippet in a new HashMap for each pathMatch instance. - .parse(pathMatch, new HashMap<>(mappingSnippet), parserContext) + // Note that a shallow copy of the map is not enough if there are multi-fields. + .parse(pathMatch, template.mappingForName(templateName, KeywordFieldMapper.CONTENT_TYPE), parserContext) .build(MapperBuilderContext.root(false, false)); extractPath(routingPaths, mapper); } diff --git a/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProviderTests.java b/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProviderTests.java index 62d07467d5086..db0e3e5cd6258 100644 --- a/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProviderTests.java +++ b/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProviderTests.java @@ -493,6 +493,55 @@ public void testGenerateRoutingPathFromDynamicTemplateWithMultiplePathMatchEntri assertEquals(3, routingPathList.size()); } + public void testGenerateRoutingPathFromDynamicTemplateWithMultiplePathMatchEntriesMultiFields() throws Exception { + Instant now = Instant.now().truncatedTo(ChronoUnit.SECONDS); + String mapping = """ + { + "_doc": { + "dynamic_templates": [ + { + "labels": { + "path_match": ["xprometheus.labels.*", "yprometheus.labels.*"], + "mapping": { + "type": "keyword", + "time_series_dimension": true, + "fields": { + "text": { + "type": "text" + } + } + } + } + } + ], + "properties": { + "host": { + "properties": { + "id": { + "type": "keyword", + "time_series_dimension": true + } + } + }, + "another_field": { + "type": "keyword" + } + } + } + } + """; + Settings result = generateTsdbSettings(mapping, now); + assertThat(result.size(), equalTo(3)); + assertThat(IndexSettings.TIME_SERIES_START_TIME.get(result), equalTo(now.minusMillis(DEFAULT_LOOK_BACK_TIME.getMillis()))); + assertThat(IndexSettings.TIME_SERIES_END_TIME.get(result), equalTo(now.plusMillis(DEFAULT_LOOK_AHEAD_TIME.getMillis()))); + assertThat( + IndexMetadata.INDEX_ROUTING_PATH.get(result), + containsInAnyOrder("host.id", "xprometheus.labels.*", "yprometheus.labels.*") + ); + List routingPathList = IndexMetadata.INDEX_ROUTING_PATH.get(result); + assertEquals(3, routingPathList.size()); + } + public void testGenerateRoutingPathFromDynamicTemplate_templateWithNoPathMatch() throws Exception { Instant now = Instant.now().truncatedTo(ChronoUnit.SECONDS); String mapping = """ From 9e9076fd71acf6e68d395485b2e2d777c1d91e3d Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 16 Jan 2024 16:27:21 +0100 Subject: [PATCH 2/5] Update docs/changelog/104418.yaml --- docs/changelog/104418.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/104418.yaml diff --git a/docs/changelog/104418.yaml b/docs/changelog/104418.yaml new file mode 100644 index 0000000000000..d27b66cebea87 --- /dev/null +++ b/docs/changelog/104418.yaml @@ -0,0 +1,6 @@ +pr: 104418 +summary: Fix `routing_path` when template has multiple `path_match` and multi-fields +area: TSDB +type: bug +issues: + - 104400 From 56ec4967f3f00ee2795b8a37c535c874e94094ed Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 16 Jan 2024 16:38:12 +0100 Subject: [PATCH 3/5] Fix code comment --- .../datastreams/DataStreamIndexSettingsProvider.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java b/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java index 090c2ee714eed..f9067239db328 100644 --- a/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java +++ b/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java @@ -179,9 +179,9 @@ private List findRoutingPaths(String indexName, Settings allSettings, Li for (String pathMatch : template.pathMatch()) { var mapper = parserContext.typeParser(mappingSnippetType) // Since FieldMapper.parse modifies the Map passed in (removing entries for "type"), that means - // that only the first pathMatch passed in gets recognized as a time_series_dimension. To counteract - // that, we wrap the mappingSnippet in a new HashMap for each pathMatch instance. - // Note that a shallow copy of the map is not enough if there are multi-fields. + // that only the first pathMatch passed in gets recognized as a time_series_dimension. + // To avoid this, each parsing call uses a new mapping snippet. + // Note that a shallow copy of the mappingSnippet map is not enough if there are multi-fields. .parse(pathMatch, template.mappingForName(templateName, KeywordFieldMapper.CONTENT_TYPE), parserContext) .build(MapperBuilderContext.root(false, false)); extractPath(routingPaths, mapper); From a8f9bd1b66a74362244caad37fe5d36c7f4df4f0 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 16 Jan 2024 16:55:19 +0100 Subject: [PATCH 4/5] Avoid parsing mapping snippet if not necessary --- .../DataStreamIndexSettingsProvider.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java b/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java index f9067239db328..b5b6ea9c1efa5 100644 --- a/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java +++ b/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java @@ -31,8 +31,10 @@ import java.io.UncheckedIOException; import java.time.Instant; import java.util.ArrayList; +import java.util.Iterator; import java.util.List; import java.util.Locale; +import java.util.Map; import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_ROUTING_PATH; @@ -176,15 +178,18 @@ private List findRoutingPaths(String indexName, Settings allSettings, Li } MappingParserContext parserContext = mapperService.parserContext(); - for (String pathMatch : template.pathMatch()) { + for (Iterator iterator = template.pathMatch().iterator(); iterator.hasNext(); ) { var mapper = parserContext.typeParser(mappingSnippetType) + .parse(iterator.next(), mappingSnippet, parserContext) + .build(MapperBuilderContext.root(false, false)); + extractPath(routingPaths, mapper); + if (iterator.hasNext()) { // Since FieldMapper.parse modifies the Map passed in (removing entries for "type"), that means // that only the first pathMatch passed in gets recognized as a time_series_dimension. // To avoid this, each parsing call uses a new mapping snippet. // Note that a shallow copy of the mappingSnippet map is not enough if there are multi-fields. - .parse(pathMatch, template.mappingForName(templateName, KeywordFieldMapper.CONTENT_TYPE), parserContext) - .build(MapperBuilderContext.root(false, false)); - extractPath(routingPaths, mapper); + mappingSnippet = template.mappingForName(templateName, KeywordFieldMapper.CONTENT_TYPE); + } } } return routingPaths; From f04e6ee7fde0cd3e0e7b2f228089d386a640c121 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 16 Jan 2024 17:01:05 +0100 Subject: [PATCH 5/5] Apply spotless suggestions --- .../datastreams/DataStreamIndexSettingsProvider.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java b/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java index b5b6ea9c1efa5..694e015b602f8 100644 --- a/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java +++ b/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java @@ -34,7 +34,6 @@ import java.util.Iterator; import java.util.List; import java.util.Locale; -import java.util.Map; import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_ROUTING_PATH; @@ -178,7 +177,7 @@ private List findRoutingPaths(String indexName, Settings allSettings, Li } MappingParserContext parserContext = mapperService.parserContext(); - for (Iterator iterator = template.pathMatch().iterator(); iterator.hasNext(); ) { + for (Iterator iterator = template.pathMatch().iterator(); iterator.hasNext();) { var mapper = parserContext.typeParser(mappingSnippetType) .parse(iterator.next(), mappingSnippet, parserContext) .build(MapperBuilderContext.root(false, false));