Add parent-aggregation to parent-join module #34210

centic9 · 2018-10-02T05:59:32Z

This is a first try at implementing the parent-aggregation as part of the parent-join module. See #9705 for some previous discussion about this.

This is probably not fully merge-able yet, I would like to get feedback on a few points that I could not clarify myself:

For implementation I mostly do the same as the child-aggregation does with the main difference of only reporting each parent-document once as part of a bucket
Does this approach look feasible? It seems to work for our local use-cases, but there might be edge-cases that we do not trigger or detect right now.
Is there a way to do the Map<Integer, Set> in ChildrenToParentAggregator.doPostCollection() in a better way? I expect this will create quite some memory-churn. Something like IntMap from Trove or HPPC would be better, but wasn't sure if I can add a dependency on HPPC to the module.
new unit-tests and all other existing tests do pass, is there any other integration-style testing that we should add?

elasticmachine · 2018-10-02T12:08:36Z

Pinging @elastic/es-search-aggs

Kamapcuc · 2018-10-02T15:46:52Z

hooray!

jimczi

Thanks @centic9 . This is a great start ! I only checked the aggregator impl for the moment and left some comments.

...arent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregator.java

As stated in the PR, we can avoid collecting buckets into the multiple-array as long as it is the same bucket as before

We can combine both aggregators into an abstract class, only the filter needs to be defined differently. Also the lookup via seenParents is not necessary any more after the previous change.

centic9 · 2018-10-04T20:41:09Z

Refactored code to have an abstract class for similar code so the two aggregations only contain the different parts.
For Child-To-Parent I now also do a check on the existing bucket before switching to an array and thus could remove the costly lookup in the Map.

jimczi

Sorry for the late review @centic9 , I left more comments regarding the complexity and costs of this new aggregation. What do you think of restricting the new aggregation to the root level like @martijnvg proposed ? The memory and complexity required to register all buckets in the children => parent case seems really prohibitive and I think that the majority of use cases would be fine using it as the root aggregation.

...nt-join/src/main/java/org/elasticsearch/join/aggregations/AbstractParentChildAggregator.java

...arent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregator.java

This commit adds a new ParentJoinAggregator that implements a join using global ordinals in a way that can be reused by the `children` and the upcoming `parent` aggregation. This new aggregator is a refactor of the existing ParentToChildrenAggregator with two main changes: * It uses a dense bit array instead of a long array when the aggregation does not have any parent. * It uses a single aggregator per bucket if it is nested under another aggregation. For the latter case we use a `MultiBucketAggregatorWrapper` in the factory in order to ensure that each instance of the aggregator handles a single bucket. This is more inlined with the strategy we use for other aggregations like `terms` aggregation for instance since the number of buckets to handle should be low (thanks to the breadth_first strategy). This change is also required for elastic#34210 which adds the `parent` aggregation in the parent-join module. Relates elastic#34508

) This commit adds a new ParentJoinAggregator that implements a join using global ordinals in a way that can be reused by the `children` and the upcoming `parent` aggregation. This new aggregator is a refactor of the existing ParentToChildrenAggregator with two main changes: * It uses a dense bit array instead of a long array when the aggregation does not have any parent. * It uses a single aggregator per bucket if it is nested under another aggregation. For the latter case we use a `MultiBucketAggregatorWrapper` in the factory in order to ensure that each instance of the aggregator handles a single bucket. This is more inlined with the strategy we use for other aggregations like `terms` aggregation for instance since the number of buckets to handle should be low (thanks to the breadth_first strategy). This change is also required for #34210 which adds the `parent` aggregation in the parent-join module. Relates #34508

jimczi

@centic9 I merged #34845 so you should be able to use the new ParentJoinAggregator to implement the parent aggregator. It should be identical to the ParentToChildrenAggregator except that the inQuery and outQuery should be inversed.
Can you merge master and apply the changes in your code ? Also can you change the title of the pr to reflect the status (we're not in the first steps anymore ;) ).

…aggregation

The required changes are now even less as the new base class ParentJoinAggregator implements collecting buckets in a different way.

centic9 · 2018-10-26T21:41:14Z

Thanks for all this preparatory work! I have now updated the parent-aggregation to make use of the new code. Mostly removing code, test still pass locally.

) This commit adds a new ParentJoinAggregator that implements a join using global ordinals in a way that can be reused by the `children` and the upcoming `parent` aggregation. This new aggregator is a refactor of the existing ParentToChildrenAggregator with two main changes: * It uses a dense bit array instead of a long array when the aggregation does not have any parent. * It uses a single aggregator per bucket if it is nested under another aggregation. For the latter case we use a `MultiBucketAggregatorWrapper` in the factory in order to ensure that each instance of the aggregator handles a single bucket. This is more inlined with the strategy we use for other aggregations like `terms` aggregation for instance since the number of buckets to handle should be low (thanks to the breadth_first strategy). This change is also required for #34210 which adds the `parent` aggregation in the parent-join module. Relates #34508

jimczi

Sorry @centic9 I was out last week. I left some comments but we're getting close. Can you also add an entry in the documentation next to https://github.com/elastic/elasticsearch/blob/master/docs/reference/aggregations/bucket/children-aggregation.asciidoc ?

...arent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregator.java

modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentIT.java

Adjust import/javadoc, remove commented code/TODOs

…aggregation

centic9 · 2018-11-07T19:10:59Z

Hi Jim, I have now fixed the TODOs in the tests, added docu and merged in latest master to resolve a conflict in one test-case, so should be good to go from my side now, let me know if anything is still missing or incorrect :)

jimczi · 2018-11-08T08:18:10Z

test this please

jimczi

Thanks @centic9, the pr looks good. Can you change the title ?
I triggered a CI build to check if the tests pass, I'll monitor the result.

jimczi · 2018-11-08T09:17:48Z

The failure is not related to this pr. Let's try another one.

jimczi · 2018-11-08T09:17:57Z

test this please

jimczi

Thanks @centic9, tests pass ! I'll merge the pr in master and 6x

centic9 · 2018-11-08T13:36:01Z

Perfect, nice that we get this into 6.x as well!

Add `parent` aggregation, a special single bucket aggregation that joins children documents to their parent.

Relates: elastic/elasticsearch#34210 This commit adds Parent Aggregation to the high level client.

Relates: elastic/elasticsearch#34210 This commit adds Parent Aggregation to the high level client. (cherry picked from commit 1051a57)

First steps for adding Parent-Aggregation to parent-join module

b6e186c

centic9 mentioned this pull request Oct 2, 2018

[feature request] Parent aggregation #9705

Closed

javanna added the :Analytics/Aggregations Aggregations label Oct 2, 2018

javanna added >enhancement review labels Oct 2, 2018

jimczi reviewed Oct 3, 2018

View reviewed changes

...arent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregator.java Outdated Show resolved Hide resolved

...arent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregator.java Outdated Show resolved Hide resolved

jimczi reviewed Oct 4, 2018

View reviewed changes

...arent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregator.java Outdated Show resolved Hide resolved

centic9 added 2 commits October 4, 2018 20:04

Add check to not collect buckets multiple times if not needed

a86a159

As stated in the PR, we can avoid collecting buckets into the multiple-array as long as it is the same bucket as before

Combine parent-join aggregator functionality and remove seenParents

fbcf9b0

We can combine both aggregators into an abstract class, only the filter needs to be defined differently. Also the lookup via seenParents is not necessary any more after the previous change.

rjernst removed the review label Oct 10, 2018

jimczi reviewed Oct 15, 2018

View reviewed changes

...nt-join/src/main/java/org/elasticsearch/join/aggregations/AbstractParentChildAggregator.java Outdated Show resolved Hide resolved

...arent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregator.java Outdated Show resolved Hide resolved

jimczi mentioned this pull request Oct 25, 2018

Refactor children aggregator into a generic ParentJoinAggregator #34845

Merged

jimczi reviewed Oct 26, 2018

View reviewed changes

jimczi added v7.0.0 v6.6.0 labels Oct 26, 2018

centic9 added 2 commits October 26, 2018 21:41

Merge remote-tracking branch 'origin/master' into parent_join_parent_…

6e6edf7

…aggregation

Rework parent-aggregation after work in PR elastic#34845

b73af5a

The required changes are now even less as the new base class ParentJoinAggregator implements collecting buckets in a different way.

jimczi reviewed Nov 5, 2018

View reviewed changes

centic9 added 4 commits November 6, 2018 23:34

Adjust tests for parent-aggregation some more based on review-comments

01aa7d0

Adjust import/javadoc, remove commented code/TODOs

Abstract base class for tests needs to be named ...TestCase

769ad0f

Add documentation for parent-aggregation similar to children-aggregation

d37af45

Merge remote-tracking branch 'origin/master' into parent_join_parent_…

0fec20a

…aggregation

jimczi reviewed Nov 8, 2018

View reviewed changes

centic9 changed the title ~~First steps for adding Parent-Aggregation to parent-join module~~ Add parent-aggregation to parent-join module Nov 8, 2018

jimczi approved these changes Nov 8, 2018

View reviewed changes

jimczi merged commit d351422 into elastic:master Nov 8, 2018

centic9 deleted the parent_join_parent_aggregation branch November 8, 2018 13:35

jimczi pushed a commit that referenced this pull request Nov 8, 2018

Add parent-aggregation to parent-join module (#34210)

20b1690

Add `parent` aggregation, a special single bucket aggregation that joins children documents to their parent.

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Mar 18, 2019

Add Parent Aggregation

3ec43a3

Relates: elastic/elasticsearch#34210 This commit adds Parent Aggregation to the high level client.

russcam mentioned this pull request Mar 18, 2019

Add Parent Aggregation elastic/elasticsearch-net#3609

Merged

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Mar 19, 2019

Add Parent Aggregation (#3609)

9323c7a

Relates: elastic/elasticsearch#34210 This commit adds Parent Aggregation to the high level client.

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Mar 20, 2019

Add Parent Aggregation (#3609)

56843c7

Relates: elastic/elasticsearch#34210 This commit adds Parent Aggregation to the high level client.

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Mar 21, 2019

Add Parent Aggregation (#3609)

1051a57

Relates: elastic/elasticsearch#34210 This commit adds Parent Aggregation to the high level client.

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Apr 3, 2019

Add Parent Aggregation (#3609)

fcd824a

Relates: elastic/elasticsearch#34210 This commit adds Parent Aggregation to the high level client. (cherry picked from commit 1051a57)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parent-aggregation to parent-join module #34210

Add parent-aggregation to parent-join module #34210

centic9 commented Oct 2, 2018

elasticmachine commented Oct 2, 2018

Kamapcuc commented Oct 2, 2018

jimczi left a comment

centic9 commented Oct 4, 2018 •

edited

Loading

jimczi left a comment

jimczi left a comment •

edited

Loading

centic9 commented Oct 26, 2018

jimczi left a comment

centic9 commented Nov 7, 2018

jimczi commented Nov 8, 2018

jimczi left a comment

jimczi commented Nov 8, 2018

jimczi commented Nov 8, 2018

jimczi left a comment

centic9 commented Nov 8, 2018

Add parent-aggregation to parent-join module #34210

Add parent-aggregation to parent-join module #34210

Conversation

centic9 commented Oct 2, 2018

elasticmachine commented Oct 2, 2018

Kamapcuc commented Oct 2, 2018

jimczi left a comment

Choose a reason for hiding this comment

centic9 commented Oct 4, 2018 • edited Loading

jimczi left a comment

Choose a reason for hiding this comment

jimczi left a comment • edited Loading

Choose a reason for hiding this comment

centic9 commented Oct 26, 2018

jimczi left a comment

Choose a reason for hiding this comment

centic9 commented Nov 7, 2018

jimczi commented Nov 8, 2018

jimczi left a comment

Choose a reason for hiding this comment

jimczi commented Nov 8, 2018

jimczi commented Nov 8, 2018

jimczi left a comment

Choose a reason for hiding this comment

centic9 commented Nov 8, 2018

centic9 commented Oct 4, 2018 •

edited

Loading

jimczi left a comment •

edited

Loading