Remove aggregation's postCollect phase #64016

nik9000 · 2020-10-21T19:51:31Z

After #63811 it became clear to me that postCollect is kind of
dangerous and not all that useful. So this removes it.

The trouble with postCollect is that it all happened right after we
finished calling collect on the LeafBucketCollectors but before we
built the aggregation results. But in #63811 we found out that we can't
call postCollect on the children of parent or child aggregators
until we know which which aggregation results we're building.

So this removes postCollect and moves all of the things we did at
post-collect phase into buildAggregations or into hooks called in
those methods.

elasticmachine · 2020-10-21T19:51:33Z

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

After elastic#63811 it became clear to me that `postCollect` is kind of dangerous and not all that useful. So this removes it. The trouble with `postCollect` is that it all happened right after we finished calling `collect` on the `LeafBucketCollectors` but before we built the aggregation results. But in elastic#63811 we found out that we can't call `postCollect` on the children of `parent` or `child` aggregators until we know which *which* aggregation results we're building. So this removes `postCollect` and moves all of the things we did at post-collect phase into `buildAggregations` or into hooks called in those methods.

nik9000 · 2020-10-21T19:51:55Z

modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentJoinAggregator.java

-
-    @Override
-    protected void beforeBuildingBuckets(long[] ordsToCollect) throws IOException {
+    protected void prepareSubAggs(long[] bucketOrdsToCollect) throws IOException {


I renamed this method to make it a bit more clear what it is for.

nik9000 · 2020-10-21T19:52:50Z

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

     *        collect from this aggregation
     * @return the results for each ordinal, in the same order as the array
     *         of ordinals
     */
-    public abstract InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws IOException;


I think this is a more clear variable name. I made this change in a dead end when I was working on this change locally but I kind of like it.

nik9000 · 2020-10-21T19:53:34Z

...rc/main/java/org/elasticsearch/search/aggregations/bucket/BestBucketsDeferringCollector.java

-        if (finished == false) {
-            throw new IllegalStateException("Cannot replay yet, collection is not finished: postCollect() has not been called");
-        }
+        finishLeaf();


I'm pretty happy about how this turned out! No more finished to check on. It's just all built in now.

nik9000 · 2020-10-21T19:55:35Z

server/src/main/java/org/elasticsearch/search/profile/aggregation/ProfilingAggregator.java

@@ -124,17 +124,6 @@ public void preCollection() throws IOException {
        profiler.pollLastElement();
    }

-    @Override
-    public void postCollection() throws IOException {


This is kind of an unfortunate loss here. Now we'll end up having post_collection in the build_aggregation debug phase. I'm wondering if I can have aggregations that do "interesting" things opt in to make their own timers or something.

nik9000 · 2020-10-21T19:58:21Z

@cjcenizal is this something that you need to look at as well? It removes one of the entries in the aggs profile list because that line was "scary".

cjcenizal · 2020-10-21T20:35:19Z

@nik9000 Thanks for the ping! Which API does this affect, and what is the change? Is it in the request or the response? Could you provide an example before and after?

nik9000 · 2020-10-21T20:37:38Z

@nik9000 Thanks for the ping! Which API does this affect, and what is the change? Is it in the request or the response? Could you provide an example before and after?

Just the response. Have a look at the asciidoc file this PR changes.

cjcenizal

Doesn't look like ES UI depends on the post_collection or post_collection_count fields which are removed from the profile response in this PR.

nik9000 · 2020-10-26T13:07:14Z

Doesn't look like ES UI depends on the post_collection or post_collection_count fields which are removed from the profile response in this PR.

@cjcenizal would the UI be ok if I added fields to that list? Renamed them? I've been thinking about having a little more flexibility there.

nik9000 · 2020-10-26T15:29:45Z

run elasticsearch-ci/bwc

not-napoleon

LGTM

After elastic#63811 it became clear to me that `postCollect` is kind of dangerous and not all that useful. So this removes it. The trouble with `postCollect` is that it all happened right after we finished calling `collect` on the `LeafBucketCollectors` but before we built the aggregation results. But in elastic#63811 we found out that we can't call `postCollect` on the children of `parent` or `child` aggregators until we know which *which* aggregation results we're building. So this removes `postCollect` and moves all of the things we did at post-collect phase into `buildAggregations` or into hooks called in those methods.

After #63811 it became clear to me that `postCollect` is kind of dangerous and not all that useful. So this removes it. The trouble with `postCollect` is that it all happened right after we finished calling `collect` on the `LeafBucketCollectors` but before we built the aggregation results. But in #63811 we found out that we can't call `postCollect` on the children of `parent` or `child` aggregators until we know which *which* aggregation results we're building. So this removes `postCollect` and moves all of the things we did at post-collect phase into `buildAggregations` or into hooks called in those methods.

In #64016 I caused a bug in rare_terms where we would rewrite the current leaf and remove all hits from it, causing us to trip an assertion. This didn't happen before because previously we never rewrote the current leaf. The fix involves cleaning up the state that the deferring collector uses if we end up removing all hits. This requires some super intimate knowledge of how `BestBucketsDeferringCollector` works so I decided to move the implementation of that mergin from `MergingBucketsDeferringCollector` into `BestBucketsDeferringCollector`. This means that `MergingBucketsDeferringCollector` is pretty much empty now. So I've deprecated it and will remove it entirely in an subsequent change. Closes #64356

cjcenizal · 2020-10-29T21:37:05Z

@nik9000

would the UI be ok if I added fields to that list? Renamed them? I've been thinking about having a little more flexibility there.

From what I can see, the code builds the UI recursively and dynamically, following the structure of the Profile API response but agnostic to its contents. So you should be able to add, remove, and rename fields freely and I expect the UI to faithfully surface them.

In elastic#64016 I caused a bug in rare_terms where we would rewrite the current leaf and remove all hits from it, causing us to trip an assertion. This didn't happen before because previously we never rewrote the current leaf. The fix involves cleaning up the state that the deferring collector uses if we end up removing all hits. This requires some super intimate knowledge of how `BestBucketsDeferringCollector` works so I decided to move the implementation of that mergin from `MergingBucketsDeferringCollector` into `BestBucketsDeferringCollector`. This means that `MergingBucketsDeferringCollector` is pretty much empty now. So I've deprecated it and will remove it entirely in an subsequent change. Closes elastic#64356

nik9000 · 2020-10-30T14:08:29Z

From what I can see, the code builds the UI recursively and dynamically, following the structure of the Profile API response but agnostic to its contents. So you should be able to add, remove, and rename fields freely and I expect the UI to faithfully surface them.

🤘

That means if an aggs has an "unusual" phase it can add it rather than tucking it into the other phases. That seems useful!

In #64016 I caused a bug in rare_terms where we would rewrite the current leaf and remove all hits from it, causing us to trip an assertion. This didn't happen before because previously we never rewrote the current leaf. The fix involves cleaning up the state that the deferring collector uses if we end up removing all hits. This requires some super intimate knowledge of how `BestBucketsDeferringCollector` works so I decided to move the implementation of that mergin from `MergingBucketsDeferringCollector` into `BestBucketsDeferringCollector`. This means that `MergingBucketsDeferringCollector` is pretty much empty now. So I've deprecated it and will remove it entirely in an subsequent change. Closes #64356

The cardinality agg delays calculating stuff until just before it is needed. Before elastic#64016 it used the `postCollect` phase to do this work which was perfect for the `terms` agg but we decided that `postCollect` was dangerous because some aggs, notably the `parent` and `child` aggs need to know which children to build and they *can't* during `postCollect`. After elastic#64016 we built the cardinality agg results when we built the buckets. But we if you sort on the cardinality agg then you need to do the `postCollect` stuff in order to know which buckets to build! So you have a chicken and egg problem. Sort of. This change splits the difference by running the delayed cardinality agg stuff as soon as you *either* try to build the buckets *or* read the cardinality for use with sorting. This works, but is a little janky and feels wrong. It feels like we could make a structural fix to the way we read metric values from aggs before building the buckets that would make this sort of bug much more difficult to cause. But any sort of solution to this is a larger structural change. So this fixes the bug in the quick and janky way and we hope to do a more structural fix to the way we read metrics soon. Closes elastic#67782

The cardinality agg delays calculating stuff until just before it is needed. Before #64016 it used the `postCollect` phase to do this work which was perfect for the `terms` agg but we decided that `postCollect` was dangerous because some aggs, notably the `parent` and `child` aggs need to know which children to build and they *can't* during `postCollect`. After #64016 we built the cardinality agg results when we built the buckets. But we if you sort on the cardinality agg then you need to do the `postCollect` stuff in order to know which buckets to build! So you have a chicken and egg problem. Sort of. This change splits the difference by running the delayed cardinality agg stuff as soon as you *either* try to build the buckets *or* read the cardinality for use with sorting. This works, but is a little janky and feels wrong. It feels like we could make a structural fix to the way we read metric values from aggs before building the buckets that would make this sort of bug much more difficult to cause. But any sort of solution to this is a larger structural change. So this fixes the bug in the quick and janky way and we hope to do a more structural fix to the way we read metrics soon. Closes #67782

The cardinality agg delays calculating stuff until just before it is needed. Before elastic#64016 it used the `postCollect` phase to do this work which was perfect for the `terms` agg but we decided that `postCollect` was dangerous because some aggs, notably the `parent` and `child` aggs need to know which children to build and they *can't* during `postCollect`. After elastic#64016 we built the cardinality agg results when we built the buckets. But we if you sort on the cardinality agg then you need to do the `postCollect` stuff in order to know which buckets to build! So you have a chicken and egg problem. Sort of. This change splits the difference by running the delayed cardinality agg stuff as soon as you *either* try to build the buckets *or* read the cardinality for use with sorting. This works, but is a little janky and feels wrong. It feels like we could make a structural fix to the way we read metric values from aggs before building the buckets that would make this sort of bug much more difficult to cause. But any sort of solution to this is a larger structural change. So this fixes the bug in the quick and janky way and we hope to do a more structural fix to the way we read metrics soon. Closes elastic#67782

The cardinality agg delays calculating stuff until just before it is needed. Before #64016 it used the `postCollect` phase to do this work which was perfect for the `terms` agg but we decided that `postCollect` was dangerous because some aggs, notably the `parent` and `child` aggs need to know which children to build and they *can't* during `postCollect`. After #64016 we built the cardinality agg results when we built the buckets. But we if you sort on the cardinality agg then you need to do the `postCollect` stuff in order to know which buckets to build! So you have a chicken and egg problem. Sort of. This change splits the difference by running the delayed cardinality agg stuff as soon as you *either* try to build the buckets *or* read the cardinality for use with sorting. This works, but is a little janky and feels wrong. It feels like we could make a structural fix to the way we read metric values from aggs before building the buckets that would make this sort of bug much more difficult to cause. But any sort of solution to this is a larger structural change. So this fixes the bug in the quick and janky way and we hope to do a more structural fix to the way we read metrics soon. Closes #67782

This partially reverts #64016 and and adds #67839 and adds additional tests that would have caught issues with the changes in #64016. It's mostly Nik's code, I am just cleaning things up a bit. Co-authored-by: Nik Everett <nik9000@gmail.com>

This partially reverts #64016 and and adds #67839 and adds additional tests that would have caught issues with the changes in #64016. It's mostly Nik's code, I am just cleaning things up a bit. Co-authored-by: Nik Everett nik9000@gmail.com

This partially reverts elastic#64016 and and adds elastic#67839 and adds additional tests that would have caught issues with the changes in elastic#64016. It's mostly Nik's code, I am just cleaning things up a bit. Co-authored-by: Nik Everett nik9000@gmail.com

This partially reverts #64016 and and adds #67839 and adds additional tests that would have caught issues with the changes in #64016. It's mostly Nik's code, I am just cleaning things up a bit. Co-authored-by: Nik Everett nik9000@gmail.com

nik9000 added :Analytics/Aggregations Aggregations >refactoring v8.0.0 v7.11.0 labels Oct 21, 2020

nik9000 requested a review from not-napoleon October 21, 2020 19:51

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 21, 2020

nik9000 commented Oct 21, 2020

View reviewed changes

Better words

93db739

nik9000 force-pushed the aggregration_drop_post_collect branch from d588dda to 93db739 Compare October 21, 2020 19:57

Fixup

e9c80e8

cjcenizal approved these changes Oct 21, 2020

View reviewed changes

nik9000 mentioned this pull request Oct 26, 2020

Cardinality aggregation missing from response inconsistently #64142

Closed

Merge branch 'master' into aggregration_drop_post_collect

1e86474

nik9000 added 2 commits October 26, 2020 14:42

Merge branch 'master' into aggregration_drop_post_collect

9b8f630

Remove extra

9ea6f51

not-napoleon approved these changes Oct 28, 2020

View reviewed changes

Merge branch 'master' into aggregration_drop_post_collect

a9ae3d7

nik9000 merged commit 3af540b into elastic:master Oct 28, 2020

nik9000 added the backport pending label Oct 28, 2020

nik9000 removed the backport pending label Oct 29, 2020

nik9000 mentioned this pull request Oct 30, 2020

Fixup rare terms (backport of #64366) #64416

Merged

This was referenced Jan 21, 2021

Fix sorting terms by cardinality agg #67839

Merged

Rework fetching metrics from Aggregators so values are "always" calculated before fetching #67852

Open

nik9000 mentioned this pull request Jan 26, 2021

Fix sorting terms by cardinality agg (backport of #67839) #67991

Merged

nik9000 mentioned this pull request Jan 26, 2021

Fix sorting terms by cardinality agg (backport of #67839) #67992

Merged

imotov mentioned this pull request Feb 5, 2021

Revert "Remove aggregation's postCollect phase #68615

Merged

imotov mentioned this pull request Feb 11, 2021

[7.x] Revert "Remove aggregation's postCollect phase #68942

Merged

imotov mentioned this pull request Feb 16, 2021

[7.11] Revert "Remove aggregation's postCollect phase (#68942) #69066

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove aggregation's postCollect phase #64016

Remove aggregation's postCollect phase #64016

nik9000 commented Oct 21, 2020

elasticmachine commented Oct 21, 2020

nik9000 Oct 21, 2020

nik9000 Oct 21, 2020

nik9000 Oct 21, 2020

nik9000 Oct 21, 2020

nik9000 commented Oct 21, 2020

cjcenizal commented Oct 21, 2020

nik9000 commented Oct 21, 2020

cjcenizal left a comment

nik9000 commented Oct 26, 2020

nik9000 commented Oct 26, 2020

not-napoleon left a comment

cjcenizal commented Oct 29, 2020

nik9000 commented Oct 30, 2020

Remove aggregation's postCollect phase #64016

Remove aggregation's postCollect phase #64016

Conversation

nik9000 commented Oct 21, 2020

elasticmachine commented Oct 21, 2020

nik9000 Oct 21, 2020

Choose a reason for hiding this comment

nik9000 Oct 21, 2020

Choose a reason for hiding this comment

nik9000 Oct 21, 2020

Choose a reason for hiding this comment

nik9000 Oct 21, 2020

Choose a reason for hiding this comment

nik9000 commented Oct 21, 2020

cjcenizal commented Oct 21, 2020

nik9000 commented Oct 21, 2020

cjcenizal left a comment

Choose a reason for hiding this comment

nik9000 commented Oct 26, 2020

nik9000 commented Oct 26, 2020

not-napoleon left a comment

Choose a reason for hiding this comment

cjcenizal commented Oct 29, 2020

nik9000 commented Oct 30, 2020