TSDB: Reject the nested object fields that are configured time_series…

…_dimension (elastic#83920) At the moment we really don't know what configuring a `time_series_dimension` should *do* when there are nested documents. So, for now, we're going to disable it. One day when someone has a good idea of how it should work we can build that. But for now we don't want to guess wrong and then lock us into some annoying behavior that no one needs but we have to support for backwards compatibility reasons. Closes: elastic#83915
probakowski · Feb 23, 2022 · ec2ecc9 · ec2ecc9
1 parent 6e77123
commit ec2ecc9
Show file tree

Hide file tree

Showing 8 changed files with 126 additions and 15 deletions.
diff --git a/docs/changelog/83920.yaml b/docs/changelog/83920.yaml
@@ -0,0 +1,5 @@
+pr: 83920
+summary: "TSDB: Reject the nested object fields that are configured time_series_dimension"
+area: TSDB
+type: enhancement
+issues: []
diff --git a/...lRestTest/resources/rest-api-spec/test/tsdb/05_dimension_and_metric_in_non_tsdb_index.yml b/...lRestTest/resources/rest-api-spec/test/tsdb/05_dimension_and_metric_in_non_tsdb_index.yml
@@ -216,12 +216,13 @@ no _tsid in standard indices:
   - is_false: fields._tsid  # _tsid metadata field must not exist in non-time-series indices
 
 ---
-nested dimensions:
+no nested dimensions:
   - skip:
-      version: all
-      reason: Awaits fix https://github.com/elastic/elasticsearch/issues/83915
+      version: " - 8.1.99"
+      reason: introduced in 8.2.0
 
   - do:
+      catch: /time_series_dimension can't be configured in nested field \[nested.dim\]/
       indices.create:
           index: test
           body:
@@ -235,14 +236,3 @@ nested dimensions:
                       dim:
                         type: keyword
                         time_series_dimension: true
-
-  - do:
-      index:
-        index: test
-        refresh: true
-        body:
-          "@timestamp": "2021-04-28T18:35:24.467Z"
-          nested:
-            - dim: foo
-            - dim: bar
-            - dim: baz
diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/20_mapping.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/20_mapping.yml
@@ -312,7 +312,7 @@ nested dimensions:
       reason: message changed in 8.2.0
 
   - do:
-      catch: /cannot have nested fields when index is in \[index.mode=time_series\]/
+      catch: /time_series_dimension can't be configured in nested field \[nested.dim\]/
       indices.create:
           index: test
           body:
@@ -333,3 +333,34 @@ nested dimensions:
                       dim:
                         type: keyword
                         time_series_dimension: true
+
+---
+nested fields:
+  - skip:
+      version: " - 8.1.99"
+      reason: message changed in 8.2.0
+
+  - do:
+      catch: /cannot have nested fields when index is in \[index.mode=time_series\]/
+      indices.create:
+        index: test
+        body:
+          settings:
+            index:
+              mode: time_series
+              routing_path: [dim]
+              time_series:
+                start_time: 2021-04-28T00:00:00Z
+                end_time: 2021-04-29T00:00:00Z
+          mappings:
+            properties:
+              "@timestamp":
+                type: date
+              dim:
+                type: keyword
+                time_series_dimension: true
+              nested:
+                type: nested
+                properties:
+                  foo:
+                    type: keyword
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java
@@ -520,4 +520,13 @@ protected void indexScriptValues(
     public FieldMapper.Builder getMergeBuilder() {
         return new Builder(simpleName(), scriptCompiler, ignoreMalformedByDefault, indexCreatedVersion).dimension(dimension).init(this);
     }
+
+    @Override
+    public void doValidate(MappingLookup lookup) {
+        if (dimension && null != lookup.nestedLookup().getNestedParent(name())) {
+            throw new IllegalArgumentException(
+                TimeSeriesParams.TIME_SERIES_DIMENSION_PARAM + " can't be configured in nested field [" + name() + "]"
+            );
+        }
+    }
 }
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java
@@ -979,4 +979,13 @@ protected String contentType() {
     public FieldMapper.Builder getMergeBuilder() {
         return new Builder(simpleName(), indexAnalyzers, scriptCompiler).dimension(dimension).init(this);
     }
+
+    @Override
+    public void doValidate(MappingLookup lookup) {
+        if (dimension && null != lookup.nestedLookup().getNestedParent(name())) {
+            throw new IllegalArgumentException(
+                TimeSeriesParams.TIME_SERIES_DIMENSION_PARAM + " can't be configured in nested field [" + name() + "]"
+            );
+        }
+    }
 }
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
@@ -1501,4 +1501,13 @@ public FieldMapper.Builder getMergeBuilder() {
             .metric(metricType)
             .init(this);
     }
+
+    @Override
+    public void doValidate(MappingLookup lookup) {
+        if (dimension && null != lookup.nestedLookup().getNestedParent(name())) {
+            throw new IllegalArgumentException(
+                TimeSeriesParams.TIME_SERIES_DIMENSION_PARAM + " can't be configured in nested field [" + name() + "]"
+            );
+        }
+    }
 }
diff --git a/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java
@@ -25,6 +25,7 @@
 import java.io.UncheckedIOException;
 import java.util.Collection;
 import java.util.HashSet;
+import java.util.List;
 import java.util.function.Function;
 
 import static org.hamcrest.Matchers.containsString;
@@ -1380,4 +1381,52 @@ public void testFieldNamesIncludeInRoot() throws Exception {
         assertThat(doc.docs().get(4).get("_field_names"), nullValue());
     }
 
+    public void testNoDimensionNestedFields() {
+        {
+            Exception e = expectThrows(IllegalArgumentException.class, () -> createDocumentMapper(mapping(b -> {
+                b.startObject("nested");
+                {
+                    b.field("type", "nested");
+                    b.startObject("properties");
+                    {
+                        b.startObject("foo")
+                            .field("type", randomFrom(List.of("keyword", "ip", "long", "short", "integer", "byte")))
+                            .field("time_series_dimension", true)
+                            .endObject();
+                    }
+                    b.endObject();
+                }
+                b.endObject();
+            })));
+            assertThat(e.getMessage(), containsString("time_series_dimension can't be configured in nested field [nested.foo]"));
+        }
+
+        {
+            Exception e = expectThrows(IllegalArgumentException.class, () -> createDocumentMapper(mapping(b -> {
+                b.startObject("nested");
+                {
+                    b.field("type", "nested");
+                    b.startObject("properties");
+                    {
+                        b.startObject("other").field("type", "keyword").endObject();
+                        b.startObject("object").field("type", "object");
+                        {
+                            b.startObject("properties");
+                            {
+                                b.startObject("foo")
+                                    .field("type", randomFrom(List.of("keyword", "ip", "long", "short", "integer", "byte")))
+                                    .field("time_series_dimension", true)
+                                    .endObject();
+                            }
+                            b.endObject();
+                        }
+                        b.endObject();
+                    }
+                    b.endObject();
+                }
+                b.endObject();
+            })));
+            assertThat(e.getMessage(), containsString("time_series_dimension can't be configured in nested field [nested.object.foo]"));
+        }
+    }
 }
diff --git a/...gned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongFieldMapper.java b/...gned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongFieldMapper.java
@@ -29,6 +29,7 @@
 import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.index.mapper.MapperBuilderContext;
 import org.elasticsearch.index.mapper.MapperParsingException;
+import org.elasticsearch.index.mapper.MappingLookup;
 import org.elasticsearch.index.mapper.SimpleMappedFieldType;
 import org.elasticsearch.index.mapper.SourceValueFetcher;
 import org.elasticsearch.index.mapper.TextSearchInfo;
@@ -648,4 +649,12 @@ protected static long sortableSignedLongToUnsigned(long value) {
         return value ^ MASK_2_63;
     }
 
+    @Override
+    public void doValidate(MappingLookup lookup) {
+        if (dimension && null != lookup.nestedLookup().getNestedParent(name())) {
+            throw new IllegalArgumentException(
+                TimeSeriesParams.TIME_SERIES_DIMENSION_PARAM + " can't be configured in nested field [" + name() + "]"
+            );
+        }
+    }
 }