Add support for multi-value dimensions

modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml

@@ -1276,6 +1276,7 @@ multi value dimensions:
           - '{ "create": { "dynamic_templates": { "data": "counter_metric" } } }'
           - '{ "@timestamp": "2023-09-01T13:03:08.138Z","data": "20", "attributes": { "dim1": ["b" , "a"], "dim2": [1, 2] } }'
           - '{ "create": { "dynamic_templates": { "data": "counter_metric" } } }'
+          - '{ "@timestamp": "2023-09-01T13:03:08.138Z","data": "20", "attributes": { "dim1": ["c" , "b"], "dim2": [1, 2] } }'
   - is_false: errors
 
   - do:
@@ -1288,7 +1289,7 @@ multi value dimensions:
               terms:
                 field: _tsid
 
-  - length: { aggregations.tsids.buckets: 2 } # only the order of the dim1 attribute is different, yet we expect to have two distinct time series
+  - length: { aggregations.tsids.buckets: 3 } # only the order of the dim1 attribute is different, yet we expect to have two distinct time series
 
   - do:
       search:
@@ -1299,9 +1300,13 @@ multi value dimensions:
             dims:
               terms:
                 field: dim1
+                order:
+                  _key: asc
 
-  - length: { aggregations.dims.buckets: 2 }
+  - length: { aggregations.dims.buckets: 3 }
   - match: { aggregations.dims.buckets.0.key: a }
   - match: { aggregations.dims.buckets.0.doc_count: 2 }
   - match: { aggregations.dims.buckets.1.key: b }
-  - match: { aggregations.dims.buckets.1.doc_count: 2 }
+  - match: { aggregations.dims.buckets.1.doc_count: 3 }
+  - match: { aggregations.dims.buckets.2.key: c }
+  - match: { aggregations.dims.buckets.2.doc_count: 1 }

rest-api-spec/build.gradle

@@ -53,3 +53,7 @@ tasks.register('enforceYamlTestConvention').configure {
 tasks.named("precommit").configure {
   dependsOn 'enforceYamlTestConvention'
 }
+
+tasks.named("yamlRestCompatTestTransform").configure({ task ->
+  task.skipTest("tsdb/140_routing_path/multi-value routing path field", "Multi-value routing paths are allowed now. See #112645")
+})

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/140_routing_path.yml

@@ -119,11 +119,11 @@ missing dimension on routing path field:
                 type: keyword
 
 ---
-multi-value routing path field:
+multi-value routing path field succeeds:
   - requires:
       test_runner_features: close_to
-      cluster_features: ["gte_v8.13.0", "routing.multi_value_routing_path"]
-      reason: _tsid hashing introduced in 8.13
+      cluster_features: ["routing.multi_value_routing_path"]
+      reason: support for multi-value dimensions
 
   - do:
       indices.create:

server/src/main/java/org/elasticsearch/cluster/routing/IndexRouting.java

@@ -302,19 +302,17 @@ public String createId(Map<String, Object> flat, byte[] suffix) {
             for (Map.Entry<String, Object> e : flat.entrySet()) {
                 if (isRoutingPath.test(e.getKey())) {
                     if (e.getValue() instanceof List<?> listValue) {
-                        listValue.forEach(v -> hashValue(b, e.getKey(), v));
+                        for (Object v : listValue) {
+                            b.addHash(e.getKey(), new BytesRef(v.toString()));
+                        }
                     } else {
-                        hashValue(b, e.getKey(), e.getValue());
+                        b.addHash(e.getKey(), new BytesRef(e.getValue().toString()));
                     }
                 }
             }
             return b.createId(suffix, IndexRouting.ExtractFromSource::defaultOnEmpty);
         }
 
-        private static void hashValue(Builder b, String key, Object value) {
-            b.hashes.add(new NameAndHash(new BytesRef(key), hash(new BytesRef(value.toString()))));
-        }
-
         private static int defaultOnEmpty() {
             throw new IllegalArgumentException("Error extracting routing: source didn't contain any routing fields");
         }
@@ -344,7 +342,7 @@ public class Builder {
 
             public void addMatching(String fieldName, BytesRef string) {
                 if (isRoutingPath.test(fieldName)) {
-                    hashes.add(new NameAndHash(new BytesRef(fieldName), hash(string)));
+                    addHash(fieldName, string);
                 }
             }
 
@@ -382,7 +380,7 @@ private void extractItem(String path, XContentParser source) throws IOException
                     case VALUE_STRING:
                     case VALUE_NUMBER:
                     case VALUE_BOOLEAN:
-                        hashes.add(new NameAndHash(new BytesRef(path), hash(new BytesRef(source.text()))));
+                        addHash(path, new BytesRef(source.text()));
                         source.nextToken();
                         break;
                     case START_ARRAY:
@@ -402,6 +400,10 @@ private void extractItem(String path, XContentParser source) throws IOException
                 }
             }
 
+            private void addHash(String path, BytesRef value) {
+                hashes.add(new NameAndHash(new BytesRef(path), hash(value), hashes.size()));
+            }
+
             private int buildHash(IntSupplier onEmpty) {
                 if (hashes.isEmpty()) {
                     return onEmpty.getAsInt();
@@ -470,10 +472,13 @@ private String error(String operation) {
         }
     }
 
-    private record NameAndHash(BytesRef name, int hash) implements Comparable<NameAndHash> {
+    private record NameAndHash(BytesRef name, int hash, int order) implements Comparable<NameAndHash> {
         @Override
         public int compareTo(NameAndHash o) {
-            return name.compareTo(o.name);
+            int i = name.compareTo(o.name);
+            if (i != 0) return i;
+            // ensures array values are in the order as they appear in the source
+            return Integer.compare(order, o.order);
         }
     }
 }

server/src/main/java/org/elasticsearch/index/mapper/TimeSeriesIdFieldMapper.java

@@ -179,7 +179,7 @@ public static class TimeSeriesIdBuilder implements DocumentDimensions {
         private final Murmur3Hasher tsidHasher = new Murmur3Hasher(0);
 
         /**
-         * A sorted set of the serialized values of dimension fields that will be used
+         * A map of the serialized values of dimension fields that will be used
          * for generating the _tsid field. The map will be used by {@link TimeSeriesIdFieldMapper}
          * to build the _tsid field for the document.
          */
@@ -205,6 +205,7 @@ public BytesReference buildLegacyTsid() throws IOException {
                     out.writeBytesRef(entry.getKey());
                     List<BytesReference> value = entry.getValue();
                     if (value.size() > 1) {
+                        // multi-value dimensions are only supported for newer indices that use buildTsidHash
                         throw new IllegalArgumentException(
                             "Dimension field [" + entry.getKey().utf8ToString() + "] cannot be a multi-valued field."
                         );
@@ -271,7 +272,9 @@ public BytesReference buildTsidHash() {
             // NOTE: hash all dimension field allValues
             tsidHasher.reset();
             for (final List<BytesReference> values : dimensions.values()) {
-                values.forEach(v -> tsidHasher.update(v.toBytesRef().bytes));
+                for (BytesReference v : values) {
+                    tsidHasher.update(v.toBytesRef().bytes);
+                }
             }
             tsidHashIndex = writeHash128(tsidHasher.digestHash(), tsidHash, tsidHashIndex);
 
@@ -378,10 +381,15 @@ private void add(String fieldName, BytesReference encoded) throws IOException {
             List<BytesReference> values = dimensions.get(name);
             if (values == null) {
                 // optimize for the common case where dimensions are not multi-valued
-                values = new ArrayList<>(1);
-                values.add(encoded);
-                dimensions.put(name, values);
+                dimensions.put(name, List.of(encoded));
             } else {
+                if (values.size() == 1) {
+                    // converts the immutable list that's optimized for the common case of having only one value to a mutable list
+                    BytesReference previousValue = values.get(0);
+                    values = new ArrayList<>(4);
+                    values.add(previousValue);
+                    dimensions.put(name, values);
+                }
                 values.add(encoded);
             }
         }

server/src/test/java/org/elasticsearch/cluster/routing/IndexRoutingTests.java

@@ -601,7 +601,8 @@ public void testRoutingPathArraysInSource() throws IOException {
         IndexRouting routing = indexRoutingForPath(shards, "a,b,c,d");
         assertIndexShard(
             routing,
-            Map.of("a", List.of("foo", "bar", "foo"), "b", List.of(21, 42), "c", List.of(true), "d", List.of()),
+            Map.of("c", List.of(true), "d", List.of(), "a", List.of("foo", "bar", "foo"), "b", List.of(21, 42)),
+            // Note that the fields are sorted
             Math.floorMod(hash(List.of("a", "foo", "a", "bar", "a", "foo", "b", "21", "b", "42", "c", "true")), shards)
         );
     }