Make a builder for _tsid by nik9000 · Pull Request #83799 · elastic/elasticsearch

nik9000 · 2022-02-10T18:37:00Z

_tsid is built by getting a sorted list of encoded dimensions. This
creates a TimeSeriesIdBuilder that abstracts that behind sensibly
named methods.

elasticmachine · 2022-02-10T18:37:03Z

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

nik9000 · 2022-02-10T18:37:34Z

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

-                ? TimeSeriesIdFieldMapper.encodeTsidValue(NetworkAddress.format(address))
-                : null;
-            context.doc().addDimensionBytes(fieldType().name(), bytes);
+            context.doc().getDimensions().addIp(fieldType().name(), address);


This gets decided when the LuceneDocument is built - no need to check.

nik9000 · 2022-02-10T18:38:25Z

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

-            return decodeTsid(input);
-        } catch (IOException ex) {
-            throw new IllegalArgumentException("Dimension field cannot be deserialized.", ex);
+    public static class TimeSeriesIdBuilder implements DocumentDimensions {


I stuffed this as an inner class because I have more follow up plans that I've half written that'd move it and this seemed like the most convenient temporary spot.

`_tsid` is built by getting a sorted list of encoded dimensions. This creates a `TimeSeriesIdBuilder` that abstracts that behind sensibly named methods.

imotov

These changes exceeded my expectations of potential benefits from this refactoring. I really like how it consolidates the tsid-related logic together and where it is going. I added a couple of small suggestions, but otherwise it looks awesome!

imotov · 2022-02-11T01:45:45Z

server/src/main/java/org/elasticsearch/index/IndexMode.java

+
+        @Override
+        public DocumentDimensions buildDimensionConsumer() {
+            return new DocumentDimensions.OnlySingleValueAllowed();


I really like this, but is this a breaking change?

That's how it always worked.

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

imotov · 2022-02-11T01:56:46Z

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

-    private SortedMap<BytesRef, BytesReference> dimensionBytes;

-    LuceneDocument(String path, LuceneDocument parent) {
+    LuceneDocument(String path, LuceneDocument parent, DocumentDimensions dimensions) {


Does it need to be a part of lucene document?

Hmmmmmm........ It needs to be part of the DocumentParserContext. I suppose it makes more sense for it to live there!

It's a shame that we don't pass DocumentParserContext in the LuceneDocument. Injecting the DocumentDimensions object feels a bit weird here.

I broke it out of LuceneDocument and it is much nicer now.

imotov · 2022-02-11T01:59:07Z

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

            return this;
        }
-        final LuceneDocument doc = new LuceneDocument(fullPath, doc());
+        final LuceneDocument doc = new LuceneDocument(fullPath, doc(), doc().getDimensions());


Does it mean that nested documents can contribute dimensions into the root document? I don't think we should allow it.

A couple of things:

Nested fields aren't allowed at all in time series mode. I've opened up TSDB: improve error message for nested fields #83837 to add a test for this.

Nested fields are allowed to have dimensions currently. And it'd be breaking to change that. I think these are documented as experimental so maybe we can? But I don't want to change it in a refactoring PR. TSDB: improve error message for nested fields #83837 adds a test for the behavior we have.

imotov · 2022-02-11T02:18:19Z

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

-            return timeSeriesId;
-        }
+        TimeSeriesIdBuilder timeSeriesIdBuilder = (TimeSeriesIdBuilder) context.doc().getDimensions();
+        context.doc().add(new SortedDocValuesField(fieldType().name(), timeSeriesIdBuilder.build().toBytesRef()));


I think I would either 1) move this logic to LuceneDocument and teach it how to create tsid or 2) move the builder to context and keep LuceneDocument blissfully unaware of time series world otherwise this "Please hold this for me" type of interaction with LuceneDocument is a bit weird. I think I would prefer 2). The first solution would make more sense if we wanted to do keep different dimensions for nested documents, but at the moment they share the same set of dimensions anyway (which is probably a topic for another discussion).

I'll move it.

It's so much cleaner!

nik9000 · 2022-02-11T18:46:31Z

run elasticsearch-ci/part-2

csoulios

LGTM

csoulios · 2022-02-11T14:58:45Z

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

-    private SortedMap<BytesRef, BytesReference> dimensionBytes;

-    LuceneDocument(String path, LuceneDocument parent) {
+    LuceneDocument(String path, LuceneDocument parent, DocumentDimensions dimensions) {


It's a shame that we don't pass DocumentParserContext in the LuceneDocument. Injecting the DocumentDimensions object feels a bit weird here.

csoulios · 2022-02-14T13:11:59Z

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

-                ? TimeSeriesIdFieldMapper.encodeTsidValue(NetworkAddress.format(address))
-                : null;
-            context.doc().addDimensionBytes(fieldType().name(), bytes);
+            context.getDimensions().addIp(fieldType().name(), address);


A lot cleaner this way. Happy to see the conditional leave 👍

imotov

I really like where it is going. Thanks for iterating on this.

nik9000 · 2022-02-14T23:00:56Z

Thanks @imotov and @csoulios !

Like ships passing in the night #83799 and #83837 missed eachother and then crashed together in the master branch causing failures. Ooops.

nik9000 added >non-issue :StorageEngine/TSDB You know, for Metrics v8.2.0 labels Feb 10, 2022

nik9000 requested review from csoulios and imotov February 10, 2022 18:37

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 10, 2022

nik9000 commented Feb 10, 2022

View reviewed changes

nik9000 added 2 commits February 10, 2022 13:45

Make a builder for _tsid

be10d63

`_tsid` is built by getting a sorted list of encoded dimensions. This creates a `TimeSeriesIdBuilder` that abstracts that behind sensibly named methods.

Less silly

f119e88

imotov reviewed Feb 11, 2022

View reviewed changes

nik9000 added 6 commits February 11, 2022 09:43

Merge branch 'master' into tsdb_builder

6b20062

Move it!

f29ec5c

I don't need this any more

a407960

Stop failing

eab5921

Merge branch 'master' into tsdb_builder

4e141cb

Sneaky

8f13451

nik9000 mentioned this pull request Feb 11, 2022

TSDB: Support GET and DELETE and doc versioning #82633

Merged

csoulios approved these changes Feb 14, 2022

View reviewed changes

nik9000 mentioned this pull request Feb 14, 2022

TSDB: unicode dimensions #83681

Merged

imotov approved these changes Feb 14, 2022

View reviewed changes

nik9000 merged commit da6a1e4 into elastic:master Feb 14, 2022

nik9000 added a commit that referenced this pull request Feb 14, 2022

Skip broken test

5863038

Like ships passing in the night #83799 and #83837 missed eachother and then crashed together in the master branch causing failures. Ooops.

weizijun mentioned this pull request Feb 15, 2022

TSDB: Reject the nested object fields that are configured time_series_dimension #83920

Merged

Conversation

nik9000 commented Feb 10, 2022

Uh oh!

elasticmachine commented Feb 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imotov 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csoulios Feb 11, 2022 • 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!

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 commented Feb 11, 2022

Uh oh!

csoulios left a comment

Choose a reason for hiding this comment

Uh oh!

csoulios Feb 11, 2022 • 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!

imotov left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Feb 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

csoulios Feb 11, 2022 •

edited

Loading

csoulios Feb 11, 2022 •

edited

Loading