TSDB: Add support for composite aggregation on `_tsid` field by csoulios · Pull Request #81998 · elastic/elasticsearch

csoulios · 2021-12-21T15:30:34Z

Currently, composite aggregation supports only string or numeric values for the fields in the sources part of the composite aggregation. Although _tsid is encoded and stored as a byte array, it is formatted as map {"dim1": "value1", "dim2": value2", ...} for input/output.

This PR adds support for composite aggregation on _tsid field.

Relates to #74660

elasticmachine · 2021-12-21T15:30:37Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

elasticmachine · 2021-12-21T15:30:37Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

csoulios · 2021-12-21T19:18:29Z

...java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregationBuilder.java

+                } else if (obj instanceof Map<?, ?> && configs[i].fieldType().getClass() == TimeSeriesIdFieldType.class) {
+                    // If input is a _tsid map, encode the map to the _tsid BytesRef
+                    // TODO: Must add validations
+                    values[i] = configs[i].format().parseBytesRef(obj);


In the after part of the query, the _tsid field is passed as a Map.

Here we encode the Map to its BytesRef representation. From this point on, we treat the tsid after key as a binary value

Another option could be to encode the json map in a string ? These values are not formatted for visualizations, they are part of the after key so the content an be obfuscated.

We could base64 encode the tsid and slap it in the string. That'd obfuscate it plenty. I didn't like that idea before Christmas but I can't really remember why now....

For things like search_after I think it's nice to have a readable after key. That we're formatting dates in composite makes me think we want to format tsid - and the map is the formatted version. But if we just need some key - the base64 encoded version is easier.

server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java

jimczi

The approach makes sense, another option would be to consider the _tsid as an encoded string. The benefit of the navigable map is to expose all dimensions individually so I understand why it is preferred. Although, it should be the responsibility of the _tsid mapper to validate that the map is parseable.

jimczi · 2021-12-22T13:57:01Z

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/100_composite.yml

+  - skip:
+      version: " - 8.0.99"
+      reason: introduced in 8.1.0
+


Since it's a new field type, I'd recommend to also add unit tests in CompositeAggregatorTests for coverage.

nik9000 · 2021-12-27T15:27:50Z

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/100_composite.yml

+  - skip:
+      version: " - 8.0.99"
+      reason: introduced in 8.1.0
+


nik9000 · 2021-12-27T17:00:50Z

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

        SortedMap<String, BytesReference> dimensionFields = context.doc().getDimensionBytes();
-        if (dimensionFields.isEmpty()) {
+        BytesReference timeSeriesId = buildTsidField(dimensionFields);
+        assert timeSeriesId != null : "In time series mode _tsid cannot be null";


I don't think the method can really return null anyway. Maybe not worth checking.

nik9000 · 2021-12-27T17:06:19Z

...java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregationBuilder.java

+                } else if (obj instanceof Map<?, ?> && configs[i].fieldType().getClass() == TimeSeriesIdFieldType.class) {
+                    // If input is a _tsid map, encode the map to the _tsid BytesRef
+                    // TODO: Must add validations
+                    values[i] = configs[i].format().parseBytesRef(obj);


We could base64 encode the tsid and slap it in the string. That'd obfuscate it plenty. I didn't like that idea before Christmas but I can't really remember why now....

nik9000 · 2021-12-27T17:07:45Z

...java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregationBuilder.java

+                } else if (obj instanceof Map<?, ?> && configs[i].fieldType().getClass() == TimeSeriesIdFieldType.class) {
+                    // If input is a _tsid map, encode the map to the _tsid BytesRef
+                    // TODO: Must add validations
+                    values[i] = configs[i].format().parseBytesRef(obj);


For things like search_after I think it's nice to have a readable after key. That we're formatting dates in composite makes me think we want to format tsid - and the map is the formatted version. But if we just need some key - the base64 encoded version is easier.

nik9000

LGTM, though I pinged @romseygeek for a second opinion And! Can you look at search_after? It's quite related to composite and it probably should also "just work" after this PR. Or, at least, it should be close.

nik9000 · 2022-01-06T14:57:41Z

server/src/main/java/org/elasticsearch/search/DocValueFormat.java

+
+        @Override
+        public BytesRef parseBytesRef(Object value) {
+            if (value instanceof Map<?, ?> m) {


I think an early return would be more readable here, even if you have to sacrifice the pattern matching. I don't know enough about Java's pattern matching to know if you have to though.

Added an early return and a cast to Map to make it more readable. Dropped the pattern matching

nik9000 · 2022-01-18T20:15:28Z

server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java

@@ -199,7 +200,12 @@ private static Object convertValueFromSortType(String fieldName, SortField.Type

                case STRING_VAL:
                case STRING:


Oh boy. This is a funny case where STRING actually means "sort by a byte array. Oh yeah, and usually that's utf-8 encoded string, but not always". Yikes. Sad Nik.

But I don't think there is much you can do about it because its a lucene concept.

@romseygeek may be able to double check on this. I think it's right, but still sad.

BytesRefs compare byte-by-byte, no need to worry about UTF-8 encoding or whatever.

TSID is stored as a SortedSetDocValuesField, so what will actually get compared here is their ordinal number - SSDV is stored as a terms dictionary containing all the (sorted and deduplicated) values, and then a per-document ordinal. So to retrieve the value you get the ord for the current doc and then get the value from the terms dictionary, but to sort on it you can just compare their ordinal values - much faster than comparing byte by byte.

what will actually get compared here is their ordinal number

Yeah. I should have said "sort as though by byte array" - it's using ordinals. The thing that bothers me is that it uses the word STRING here when it means BYTES. But I'm being grumbly.

csoulios added 2 commits December 15, 2021 13:51

Add support for composite agg on _tsid

328b04f

Added support for "after"

9048fbf

csoulios added :Analytics/CompositeAggs :StorageEngine/TSDB You know, for Metrics v8.1.0 labels Dec 21, 2021

elasticmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Dec 21, 2021

csoulios added the >non-issue label Dec 21, 2021

csoulios added 3 commits December 21, 2021 17:42

Fix build error

866e623

Checkstyle

eeab826

Merge branch 'master' into tsdb-composite

20807d1

csoulios commented Dec 21, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java Outdated Show resolved Hide resolved

csoulios added 2 commits December 22, 2021 11:12

cleanup

25a410d

Merge branch 'master' into tsdb-composite

60e4b28

csoulios requested a review from jimczi December 22, 2021 12:59

jimczi reviewed Dec 22, 2021

View reviewed changes

csoulios mentioned this pull request Dec 22, 2021

TSDB: disable _tsid in composite aggregation #81646

Closed

csoulios changed the title ~~WIP: Add support for composite aggregation on _tsid field~~ [TSDB] Add support for composite aggregation on _tsid field Dec 22, 2021

csoulios added 2 commits December 23, 2021 15:23

Merge branch 'master' into tsdb-composite

5570006

Cleanup

8ec183d

nik9000 reviewed Dec 27, 2021

View reviewed changes

csoulios added 4 commits January 4, 2022 10:06

Merge branch 'master' into tsdb-composite

3d8a4af

Added composite agg tests

c42e1a8

Changed comment

073430e

Merge branch 'master' into tsdb-composite

d0e55ee

csoulios requested a review from nik9000 January 5, 2022 14:44

csoulios added 2 commits January 6, 2022 11:30

Merge branch 'master' into tsdb-composite

3886996

Merge branch 'master' into tsdb-composite

909a75b

nik9000 approved these changes Jan 18, 2022

View reviewed changes

csoulios added 2 commits January 19, 2022 15:42

Merge branch 'master' into tsdb-composite

41d264b

style change

6be7c13

csoulios mentioned this pull request Jan 19, 2022

Add better support for metric data types (TSDB) #74660

Closed

csoulios changed the title ~~[TSDB] Add support for composite aggregation on _tsid field~~ TSDB: Add support for composite aggregation on _tsid field Jan 19, 2022

csoulios merged commit 8ae9781 into elastic:master Jan 19, 2022

csoulios deleted the tsdb-composite branch May 3, 2022 16:08

		@@ -199,7 +200,12 @@ private static Object convertValueFromSortType(String fieldName, SortField.Type

		case STRING_VAL:
		case STRING:

Conversation

csoulios commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 21, 2021

Uh oh!

elasticmachine commented Dec 21, 2021

Uh oh!

csoulios Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

csoulios commented Dec 21, 2021 •

edited

Loading

csoulios Dec 21, 2021 •

edited

Loading