Add a flag to control in index order execution mode to aggregations #82129

imotov · 2021-12-29T18:37:02Z

Adds a flag to control in index order execution mode to aggregations and
introduces the basic time_series aggregation that triggers that mode.

Relates to #74660

Adds a flag to control in index order execution mode to aggregations and introduces the basic time_series aggregation that triggers that mode. Relates to elastic#74660

elasticmachine · 2021-12-30T16:21:44Z

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

romseygeek

I did a first pass over the lucene-specific parts and left some comments; I think we may be able to do this using a combination of CollectorManager and leaf segment sorters, but I need to think about it carefully.

server/src/main/java/org/apache/lucene/search/ConcurrentTopScoreDocCollector.java

.../src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java

jpountz · 2022-01-06T08:50:44Z

My main concern with this approach is that the Collector API has a general expectation that doc IDs get collected in global order. So reusing the Collector API as-is this way creates room for bugs since one could mistakenly use a random collector from Lucene assuming that it would work, but it actually wouldn't because documents wouldn't get collected the way it expects.

I guess there are two main ways how we could avoid this issue.

Either we could create a single-segment view of the entire index where doc ID order matches the index sort, e.g. using a combination of SlowCompositeReaderWrapper and SortingCodecReader (this would be a no-op on indices that have been force-merged). This way any existing collector would be legal since we would always collect doc IDs in global doc ID order.
Or we need to change the way that documents get collected. One sub-option is to reuse the Collector API by using a different collector per segment (which I think is @romseygeek's suggestion) so that we would still meet Lucene's expectation that doc IDs get collected in global order on a per-collector basis. And then merge information collected across all collectors to get the final result for the shard. The other sub-option is to use a different API for collection that doesn't require collection in global doc ID order (which could be just a sub-class of Collector that expands what is legal usage, this would at least guarantee that you couldn't pass a Lucene collector like TopDocsCollector as-is, the compiler would detect that there is a problem).

…-order-aggs-executor

imotov · 2022-01-11T03:21:15Z

@romseygeek, @nik9000 I have made some changes as we discussed. Could you take another look?

imotov · 2022-01-18T05:33:49Z

@nik9000 it has been a week, do you think you can take a look at some time soon?

nik9000

I found a few small things. Otherwise, it's what i expected us to do.

server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java

...pi-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/450_time_series.yml

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

nik9000 · 2022-01-18T19:13:59Z

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

+                    TimeSeriesAggregationBuilder.PARSER
+                ).addResultReader(InternalTimeSeries::new),
+                builder
+            );


Note for anyone else scanning this PR - this skips registering the agg if the feature flag is not enabled. The agg is still registered regardless of the index's mode.

nik9000 · 2022-01-18T19:15:20Z

server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java

+     */
+    public static TimeSeriesAggregationBuilder timeSeries(String name) {
+        return new TimeSeriesAggregationBuilder(name);
+    }


Do we need to keep this class around any more? I thought it was mostly for the transport client and I've been sort of ignoring it for years.

It is nice for IT tests that I really needed here to make sure that whole thing works. I am ok with removing it as a class, but we probably need a bigger discussion and do it outside of this PR.

.../src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java

nik9000 · 2022-01-18T19:30:48Z

.../src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java

+                    docId = iterator.nextDoc();
+                    if (docId != DocIdSetIterator.NO_MORE_DOCS && (liveDocs == null || liveDocs.get(docId))) {
+                        if (tsids.advanceExact(docId)) {
+                            BytesRef tsid = tsids.lookupOrd(tsids.nextOrd());


It'd be nice be able to avoid ord lookups if we matched the old one, I think. I've not dug into the lookup code in a while though. But, either way, that can wait.

I want to replace this whole thing with generic FieldComparator and index order stuff.

test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java

…-order-aggs-executor

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregator.java

jpountz · 2022-01-19T09:54:25Z

server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesAggregator.java

+            @Override
+            public void collect(int doc, long bucket) throws IOException {
+                if (tsids.advanceExact(doc)) {
+                    BytesRef newTsid = tsids.nextValue();


Let's not do value lookups on every document, since these are costly operations. We could take advantage of in-order iteration to keep track of the previous segment ordinal and the previous bucket ordinal. When the current segment ordinal is the same as the previous segment ordinal, we know that the bucket ordinal is the same as well, and we only need to call lookupOrd whenever the ordinal changes, which is bounded by the unique number of time series that exist in the segment (which should be a much smaller number than the number of docs in the segment)?

So, I would like to replace this whole part in a follow up with getting data based on the actually specified index sort order instead of hard coded tsids and timestamps. Are you ok, if I address these optimizations there?

Totally fine with a follow-up.

server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java

jpountz · 2022-01-19T10:12:21Z

.../src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java

+                docId = iterator.nextDoc();
+                if (docId != DocIdSetIterator.NO_MORE_DOCS && (liveDocs == null || liveDocs.get(docId))) {
+                    if (tsids.advanceExact(docId)) {
+                        BytesRef tsid = tsids.lookupOrd(tsids.nextOrd());


Could we avoid doing lookupOrd on each document? E.g. we could keep track of the previous ordinal and only reload the binary tsid if the ordinal is different from the previous one. Or use global ordinals.

jpountz · 2022-01-19T10:17:34Z

.../src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java

+                queue.pop();
+            }
+        }
+    }


nit: we'll probably want to optimize the case when there is a single leaf in the priority queue to do a simple for loop, which would happen if:

the time range filter is thin and matches a single segment

the shard has been force-merged

Thinking a bit deeper into this: the current implementation of this PR calls updateTop or pop on every document. But these are not cheap because they force at least one comparison, which is not free as the tsid values are arbitrary-length BytesRefs objects that could have long common prefixes.

We expect many docs to have the same TSID, so we could do better by popping the top entry of the priority queue. Then two cases:

If the priority queue is empty or the new top entry of the PQ has a different TSID, then we can iterate all documents of the current entry without doing any comparison.

If the new top entry of the PQ has the same TSID, we look at its timestamp value. And we know that we can collect all documents of the current entry until we find a timestamp that is greater than the timestamp of the top entry. And for all these documents, we only had to compare the timestamp (cheap), not the TSID.

I'd mentioned something vague about "we should use tsid's ordinals to skip the tsid comparison" which sounds like the thing you are talking about in the second point.

I'm wondering if these are "for a followup" things. They are 100% good things.

A follow-up would be totally fine with me.

As I mentioned before I would like to switch to actual index settings here and make it more generic.

.../src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java

jpountz · 2022-01-19T10:29:02Z

.../src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java

+                queue.pop();
+            }
+        }
+    }


I believe that the way we are doing search by-passes checks whether the query has been cancelled or timed out, so we'll need to add this logic to this search implementation?

That's a good point. I hadn't thought about cancelation when I was looking last.

Indeed, we will need to add this logic. I will do it as a follow up.

nik9000

I left a few tiny things. I think @jpountz's comments are more important. Though I'm not sure if they all should block merging or be important follow ups. We should totally do all the things he mentioned though.

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregator.java

server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java

nik9000 · 2022-01-19T17:37:51Z

.../src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java

+                queue.pop();
+            }
+        }
+    }


I'd mentioned something vague about "we should use tsid's ordinals to skip the tsid comparison" which sounds like the thing you are talking about in the second point.

I'm wondering if these are "for a followup" things. They are 100% good things.

nik9000 · 2022-01-19T17:39:01Z

.../src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java

+                queue.pop();
+            }
+        }
+    }


That's a good point. I hadn't thought about cancelation when I was looking last.

imotov · 2022-01-24T20:10:59Z

@elasticmachine update branch

…PR, and remove the local TimeSeriesIndexSearcher

weizijun · 2022-01-19T08:43:52Z

.../src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java

+
+/**
+ * An IndexSearcher wrapper that executes the searches in time-series indices by traversing them by tsid and timestamp
+ * TODO: Convert it to use index sort instead of hard-coded tsid and timestamp values


Note: it is now difficult to check if a sorted field is a multi-valued field, sorted multi-valued fields are based on index.sort.mode, not all values in a doc are sorted, LeafWalker maybe can not work well in this case.

Oh, I don't click summit review before, Now I start the review.

I am not sure I understand your comment.

Sorry, I describe more clearly. Now It's in the time_series sense, the index is sort by _tsid and @timestamp, and the _tsid and @timestamp must be a single value field, so it's ok.

But in a more common index sort sense, it may meet the multi-valued fields problem. e.g:
index is sort by foo, the setting is

index.sort.field = foo. index.sort.order = asc. index.sort.mode = min.

the sample doc:

doc1: {"foo":[1,3,5,7]} doc2: {"foo":[2,4,6,8]} doc3: {"foo":[3,4,5]}

As index.sort.mode = min, To get the sorted doc, the order is doc1->doc2->doc3, but doc2\doc3 have some value that are small than doc1.
so traveling the foo field is not in a real sequential way.

The issue (#80825) is doing to set the value to single-valued, when the sorted field is set to be a single-valued, it's ok to travel them sequential.

Add a flag to control in index order execution mode to aggregations

a2adf49

Adds a flag to control in index order execution mode to aggregations and introduces the basic time_series aggregation that triggers that mode. Relates to elastic#74660

imotov added >non-issue :StorageEngine/TSDB You know, for Metrics v8.1.0 labels Dec 29, 2021

imotov mentioned this pull request Dec 29, 2021

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

Closed

imotov added 2 commits December 29, 2021 11:13

Fix tests

36398d1

Fix TransformAggregationsTest

86f9f9c

imotov requested review from nik9000 and romseygeek December 30, 2021 16:21

imotov marked this pull request as ready for review December 30, 2021 16:21

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

romseygeek reviewed Jan 4, 2022

View reviewed changes

imotov added 3 commits January 10, 2022 08:34

Merge remote-tracking branch 'elastic/master' into issue-81121-add-in…

46623de

…-order-aggs-executor

Switch TimeSeriesIndexSearcher to BucketCollector

c3c0164

Fix NPE for empty context

16ee5f5

nik9000 requested changes Jan 18, 2022

View reviewed changes

imotov added 2 commits January 18, 2022 15:40

Merge remote-tracking branch 'elastic/master' into issue-81121-add-in…

9d10b4b

…-order-aggs-executor

Address review comments

082fd4d

imotov requested a review from nik9000 January 19, 2022 04:38

jpountz reviewed Jan 19, 2022

View reviewed changes

nik9000 reviewed Jan 19, 2022

View reviewed changes

Address more review comments

5380d4b

imotov requested a review from nik9000 January 20, 2022 16:58

nik9000 approved these changes Jan 20, 2022

View reviewed changes

weizijun mentioned this pull request Jan 24, 2022

[RollupV2]: make RollupAction available and improve some features #82944

Closed

elasticmachine and others added 2 commits January 24, 2022 13:11

Merge branch 'master' into issue-81121-add-in-order-aggs-executor

820305b

Fix compilation errors

ce057c5

imotov merged commit d9589cb into elastic:master Jan 24, 2022

weizijun added a commit to weizijun/elasticsearch that referenced this pull request Jan 25, 2022

PR: elastic#82129 has merged, use the TimeSeriesIndexSearcher in the …

d266d9c

…PR, and remove the local TimeSeriesIndexSearcher

mark-vieira mentioned this pull request Jan 27, 2022

[CI] TimeSeriesAggregationsIT classMethod failing #83187

Closed

weizijun reviewed Jan 27, 2022

View reviewed changes

imotov mentioned this pull request Apr 21, 2022

Add a flag to control in index order execution mode to aggregations #81121

Closed

Dosant mentioned this pull request Aug 29, 2022

[aggregations] support time_series aggregation elastic/kibana#131385

Closed

Add a flag to control in index order execution mode to aggregations #82129

Add a flag to control in index order execution mode to aggregations #82129

Uh oh!

Conversation

imotov commented Dec 29, 2021

Uh oh!

elasticmachine commented Dec 30, 2021

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpountz commented Jan 6, 2022

Uh oh!

imotov commented Jan 11, 2022

Uh oh!

imotov commented Jan 18, 2022

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

jpountz Jan 19, 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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imotov commented Jan 24, 2022

Uh oh!

jpountz Jan 19, 2022 •

edited

Loading

weizijun Jan 27, 2022 •

edited

Loading