Optimize composite aggregation based on index sorting by jimczi · Pull Request #48399 · elastic/elasticsearch

jimczi · 2019-10-23T13:51:06Z

This is a spinoff of #48130 that generalizes the proposal to allow early termination with the composite aggregation when leading sources match a prefix or the entire index sort specification.
In such case the composite aggregation can use the index sort natural order to early terminate the collection when it reaches a composite key that is greater than the bottom of the queue.
The optimization is also applicable when a query other than match_all is provided. However the optimization is deactivated for sources that match the index sort in the following cases:

Multi-valued source, in such case early termination is not possible.
missing_bucket is set to true

I retained the commit from #48130 and merged the original idea that @howardhuanghua described in the pr to early terminate even when the sort is reversed in the leading source.

…sort is applicable. Add index sort uts.

elasticmachine · 2019-10-23T13:51:08Z

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

jimczi · 2019-10-23T13:54:25Z

@howardhuanghua I couldn't push directly to your branch so I opened a new pr that retains your commit. I hope you don't mind if we continue to work here since we both have the right to push modifications. I merged the two approaches that we discussed in the original pr so I think this pr is ready for a review. We still need to update the documentation but I wanted to get your opinion first.

...rc/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java

howardhuanghua · 2019-10-23T14:43:33Z

Hi @jimczi , I am ok and thanks for your merging. It's my pleasure that I could contribute on this optimization with you together. I left a comment about the merged code, please help to check. Thank you.

...rc/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java

...st/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java

...rc/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java

jimczi · 2019-10-31T07:55:48Z

Thanks for looking @jpountz and @howardhuanghua . I pushed some commits to address your comments and added a section in the docs to explain the early termination. Could you take another look ?

howardhuanghua · 2019-10-31T13:45:17Z

...st/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java

        for (Query query : queries) {
-            executeTestCase(false, query, dataset, create, verify);
-            executeTestCase(true, query, dataset, create, verify);
+            executeTestCase(false, false, query, dataset, create, verify);


Since these two TestCase set useIndexSort as false, then it could not be early terminated. This would make testEarlyTermination() failed.

testEarlyTermination uses executeTestCase directly and sets useIndexSort to true so I think we're good here. However I found a test bug when the AssertingCodec is used, this codec wraps docvalues and that breaks DocValues#unwrapSingleton contract. So I pushed 1fa763b to suppress this code in the early termination tests.

Even suppressed codecs, it seems still got the chance to use
keyword -> {AssertingDocValuesFormat$AssertingDocValuesProducer@4968}. And testEarlyTermination would failed occasionally.

Good catch thanks, I pushed b52d5c7 to force the suppression of the Asserting codec but that means that the test is ignored when the randomization choose this codec. I need to dig a bit more to understand how we can ensure that the test always runs.

Hi @jimczi , I have checked that system properties like tests.codec, tests.docvaluesformat and tests.postingsformat default value is random in LuceneTestCase.java, AssertingCodec would be selected.

It seems we could set default Lucene80 codec directly in executeTestCase method as follow to solve the problem:

if (indexSort != null) { config.setCodec(TestUtil.getDefaultCodec()); // set codec as default to avoid AssertingCodec config.setIndexSort(indexSort); }

howardhuanghua · 2019-10-31T13:49:55Z

docs/reference/aggregations/bucket/composite-aggregation.asciidoc

+
+The `composite` aggregation can early terminate the collection under some circumstances:
+
+  * If the primary source extracts values from a numeric or a keyword field and the query matches all documents (`match_all` query).


Early terminate optimization could also be used for non match_all query?

Only if the index is sorted, yes. I also wanted to mention that we early terminate if the index is not sorted but I see how it's confusing now. I pushed 1fa763b to reword the paragraph to emphasize on index sorting first.

...st/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java

jimczi · 2019-11-12T11:54:17Z

I pushed another commit to address the remaining TODOs and the reviews. There is still one TODO left regarding how we could handle missingBucket but the pr is quite big already so I think we can handle it in a follow up.

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

howardhuanghua · 2019-11-12T12:41:06Z

Hi @jimczi , LGTM. Thanks for your great efforts to fix these issues.

howardhuanghua · 2019-11-26T11:48:02Z

Hi @jimczi, would you please help to merge this PR? Or we need further review? Thank you.

iverase

I did a first pass and left some questions. I will be doing a second pass and focus on the tests.

docs/reference/aggregations/bucket/composite-aggregation.asciidoc

...rc/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java

...st/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java

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

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

iverase

The logic makes sense to me. lgtm

jimczi · 2019-12-17T13:02:59Z

Thanks @howardhuanghua and sorry for the delay. I merged in master and added you as co-author. I'll now backport to 7.x.

Co-authored-by: Daniel Huang <danielhuang@tencent.com> This is a spinoff of #48130 that generalizes the proposal to allow early termination with the composite aggregation when leading sources match a prefix or the entire index sort specification. In such case the composite aggregation can use the index sort natural order to early terminate the collection when it reaches a composite key that is greater than the bottom of the queue. The optimization is also applicable when a query other than match_all is provided. However the optimization is deactivated for sources that match the index sort in the following cases: * Multi-valued source, in such case early termination is not possible. * missing_bucket is set to true

howardhuanghua · 2019-12-17T13:37:45Z

@jimczi It's my pleasure to co-work with you on this optimization. Thanks for for your help.

Co-authored-by: Daniel Huang <danielhuang@tencent.com> This is a spinoff of #48130 that generalizes the proposal to allow early termination with the composite aggregation when leading sources match a prefix or the entire index sort specification. In such case the composite aggregation can use the index sort natural order to early terminate the collection when it reaches a composite key that is greater than the bottom of the queue. The optimization is also applicable when a query other than match_all is provided. However the optimization is deactivated for sources that match the index sort in the following cases: * Multi-valued source, in such case early termination is not possible. * missing_bucket is set to true

Co-authored-by: Daniel Huang <danielhuang@tencent.com> This is a spinoff of elastic#48130 that generalizes the proposal to allow early termination with the composite aggregation when leading sources match a prefix or the entire index sort specification. In such case the composite aggregation can use the index sort natural order to early terminate the collection when it reaches a composite key that is greater than the bottom of the queue. The optimization is also applicable when a query other than match_all is provided. However the optimization is deactivated for sources that match the index sort in the following cases: * Multi-valued source, in such case early termination is not possible. * missing_bucket is set to true

zhuzhiqiang077 · 2021-10-09T07:36:05Z

...rc/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java

    private RoaringDocIdSet.Builder docIdSetBuilder;
    private BucketCollector deferredCollectors;

+    private boolean earlyTerminated;


提前结束

howardhuanghua and others added 8 commits October 23, 2019 15:38

optimize composite aggregation by index sorting

0fb183c

fix long ling issue

484f728

enhance comment

ea43c88

fix a reset issue

4a82e21

enhance test case

1c1af51

add reverse order test case

e279371

remove extra test case

d1f3091

Adapt optimization to handle more than one leading source when index …

618eeb8

…sort is applicable. Add index sort uts.

jimczi added >enhancement :Analytics/Aggregations Aggregations v8.0.0 v7.6.0 labels Oct 23, 2019

howardhuanghua reviewed Oct 23, 2019

View reviewed changes

...rc/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java Show resolved Hide resolved

howardhuanghua reviewed Oct 24, 2019

View reviewed changes

jpountz reviewed Oct 24, 2019

View reviewed changes

...rc/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java Outdated Show resolved Hide resolved

...rc/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java Outdated Show resolved Hide resolved

jimczi added 5 commits October 28, 2019 09:29

Merge branch 'master' into composite_sorted_index_optim

a5e7c25

address review

03141a5

Merge branch 'master' into composite_sorted_index_optim

64d6f50

Add documentation for early termination

646f965

fix comment

b43b72b

fix docs

ee0b122

howardhuanghua reviewed Oct 31, 2019

View reviewed changes

jimczi added 4 commits October 31, 2019 15:45

separate test to assert on early termination and update docs

1fa763b

unused imports

e497141

Merge branch 'master' into composite_sorted_index_optim

c38e7bc

fix suppresscodecs

b52d5c7

howardhuanghua reviewed Nov 7, 2019

View reviewed changes

...st/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java Show resolved Hide resolved

jimczi added 2 commits November 12, 2019 12:51

Merge branch 'master' into composite_sorted_index_optim

92e8d30

address review and TODO

c0c68cf

fix comments

3035b07

howardhuanghua reviewed Nov 12, 2019

View reviewed changes

...java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceConfig.java Outdated Show resolved Hide resolved

...java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceConfig.java Show resolved Hide resolved

iverase self-requested a review December 9, 2019 14:36

iverase reviewed Dec 10, 2019

View reviewed changes

jimczi added 2 commits December 12, 2019 23:10

Merge branch 'master' into composite_sorted_index_optim

5b0c3ab

address comments

95820b1

iverase approved these changes Dec 13, 2019

View reviewed changes

Merge branch 'master' into composite_sorted_index_optim

d182c6a

jimczi merged commit 804a504 into master Dec 17, 2019

jimczi deleted the composite_sorted_index_optim branch December 17, 2019 13:02

jimczi mentioned this pull request Dec 17, 2019

Optimize composite aggregation based on index sorting (#48399) #50272

Merged

jimczi mentioned this pull request Jan 20, 2020

Optimize composite aggregation based on index sorting. #48130

Closed

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

itizir mentioned this pull request Feb 18, 2020

7.6.0 composite aggregation on index-sorting fields: out-of-bounds exception #52480

Closed

jimczi mentioned this pull request Jun 9, 2020

Fix rounding composite aggs on sorted index #57867

Merged

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

zhuzhiqiang077 reviewed Oct 9, 2021

View reviewed changes


		The `composite` aggregation can early terminate the collection under some circumstances:

		* If the primary source extracts values from a numeric or a keyword field and the query matches all documents (`match_all` query).

Conversation

jimczi commented Oct 23, 2019

Uh oh!

elasticmachine commented Oct 23, 2019

Uh oh!

jimczi commented Oct 23, 2019

Uh oh!

Uh oh!

howardhuanghua commented Oct 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimczi commented Oct 31, 2019

Uh oh!

howardhuanghua Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

jimczi Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

howardhuanghua Nov 1, 2019

Choose a reason for hiding this comment

Uh oh!

jimczi Nov 1, 2019

Choose a reason for hiding this comment

Uh oh!

howardhuanghua Nov 6, 2019 • 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!

howardhuanghua Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

jimczi Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jimczi commented Nov 12, 2019

Uh oh!

Uh oh!

Uh oh!

howardhuanghua commented Nov 12, 2019

Uh oh!

howardhuanghua commented Nov 26, 2019

Uh oh!

iverase left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iverase left a comment

Choose a reason for hiding this comment

Uh oh!

jimczi commented Dec 17, 2019

Uh oh!

howardhuanghua commented Dec 17, 2019

Uh oh!

zhuzhiqiang077 Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

howardhuanghua Nov 6, 2019 •

edited

Loading