Unify precomputation of aggregations behind a common API by msfroh · Pull Request #16733 · opensearch-project/OpenSearch

msfroh · 2024-11-27T22:40:08Z

Description

We've had a series of aggregation speedups that use the same strategy: instead of iterating through documents that match the query one-by-one, we can look at a Lucene segment and compute the aggregation directly (if some particular conditions are met).

In every case, we've hooked that into custom logic that hijacks the getLeafCollector method and throws CollectionTerminatedException. This creates the illusion that we're implementing a custom LeafCollector, when really we're not collecting at all (which is the whole point).

With this refactoring, the mechanism (hijacking getLeafCollector) is moved into AggregatorBase. Aggregators that have a strategy to precompute their answer can override tryPrecomputeAggregationForLeaf, which is expected to return true if they managed to precompute.

This should also make it easier to keep track of which aggregations have precomputation approaches (since they override this method).

Related Issues

N/A

Check List

~~Functionality includes testing.~~
~~API changes companion pull request created, if applicable.~~
~~Public documentation issue/PR created, if applicable.~~

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2024-11-27T23:51:26Z

❌ Gradle check result for 4d5c32b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

server/src/main/java/org/opensearch/search/aggregations/AggregatorBase.java

sandeshkr419 · 2024-12-03T22:13:04Z

Regarding implementation of this, I have one more alternative which I think is worth discussing. How about bringing this abstraction at ContextIndexSearcher itself.

            weight = wrapWeight(weight);
            // See please https://github.com/apache/lucene/pull/964
            collector.setWeight(weight);
            leafCollector = collector.getLeafCollector(ctx);

Basically if we have pre computed aggregations already, we assign it as EarlyTerminationCollector.

So, what I'm thinking about is cases with sub-aggregations that we can pre-compute, which is highly relevant in cases of star tree pre-computation. For eg.: #16674 and if a dedicated abstraction for star-tree preCompute in ComtextIndexSearcher wopuld make more sense or not.

github-actions · 2024-12-12T20:50:38Z

✅ Gradle check result for 4d5c32b: SUCCESS

codecov · 2024-12-12T20:51:20Z

Codecov Report

Attention: Patch coverage is 82.05128% with 14 lines in your changes missing coverage. Please review.

Project coverage is 72.32%. Comparing base (cd149a9) to head (1c3c990).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
...rch/search/aggregations/metrics/MinAggregator.java	60.00%	2 Missing and 2 partials ⚠️
...ket/terms/GlobalOrdinalsStringTermsAggregator.java	84.21%	2 Missing and 1 partial ⚠️
...rch/aggregations/metrics/ValueCountAggregator.java	66.66%	2 Missing and 1 partial ⚠️
...rch/search/aggregations/metrics/MaxAggregator.java	80.00%	1 Missing and 1 partial ⚠️
...rch/search/aggregations/metrics/AvgAggregator.java	85.71%	1 Missing ⚠️
...rch/search/aggregations/metrics/SumAggregator.java	87.50%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #16733      +/-   ##
============================================
- Coverage     72.41%   72.32%   -0.09%     
- Complexity    65626    65712      +86     
============================================
  Files          5306     5319      +13     
  Lines        304927   305722     +795     
  Branches      44257    44348      +91     
============================================
+ Hits         220804   221107     +303     
- Misses        66007    66573     +566     
+ Partials      18116    18042      -74

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

msfroh · 2025-01-10T08:39:27Z

@jainankitk -- you're probably the maintainer (other than me) with the most context into this change. What do you think?

msfroh · 2025-01-29T20:07:00Z

@expani, @sandeshkr419 -- I resolved conflicts with your recent star-tree changes. Can you please take a look?

github-actions · 2025-01-29T20:43:07Z

❌ Gradle check result for 19a40cc: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

sandeshkr419 · 2025-01-29T20:45:35Z

One high level class I see missing among the metric aggregators is AvgAggregator.java which has similar pre-computations involved.

server/src/main/java/org/opensearch/search/aggregations/metrics/MaxAggregator.java

server/src/main/java/org/opensearch/search/aggregations/metrics/MinAggregator.java

Signed-off-by: Michael Froh <froh@amazon.com>

github-actions · 2025-01-30T00:14:59Z

✅ Gradle check result for caceb62: SUCCESS

Signed-off-by: Michael Froh <froh@amazon.com>

github-actions · 2025-01-30T01:34:49Z

✅ Gradle check result for 1c3c990: SUCCESS

@bowenlan-amzn

* Unify precomputation of aggregations behind a common API We've had a series of aggregation speedups that use the same strategy: instead of iterating through documents that match the query one-by-one, we can look at a Lucene segment and compute the aggregation directly (if some particular conditions are met). In every case, we've hooked that into custom logic hijacks the getLeafCollector method and throws CollectionTerminatedException. This creates the illusion that we're implementing a custom LeafCollector, when really we're not collecting at all (which is the whole point). With this refactoring, the mechanism (hijacking getLeafCollector) is moved into AggregatorBase. Aggregators that have a strategy to precompute their answer can override tryPrecomputeAggregationForLeaf, which is expected to return true if they managed to precompute. This should also make it easier to keep track of which aggregations have precomputation approaches (since they override this method). Signed-off-by: Michael Froh <froh@amazon.com> * Remove subaggregator check from CompositeAggregator Not sure why I added this, when the existing implementation didn't have it. That said, we *should* call finishLeaf() before precomputing the current leaf. Signed-off-by: Michael Froh <froh@amazon.com> * Resolve conflicts with star-tree changes Signed-off-by: Michael Froh <froh@amazon.com> * Skip precomputation when valuesSource is null Signed-off-by: Michael Froh <froh@amazon.com> * Add comment as suggested by @bowenlan-amzn Signed-off-by: Michael Froh <froh@amazon.com> --------- Signed-off-by: Michael Froh <froh@amazon.com> (cherry picked from commit 2847695) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…7197) We've had a series of aggregation speedups that use the same strategy: instead of iterating through documents that match the query one-by-one, we can look at a Lucene segment and compute the aggregation directly (if some particular conditions are met). In every case, we've hooked that into custom logic hijacks the getLeafCollector method and throws CollectionTerminatedException. This creates the illusion that we're implementing a custom LeafCollector, when really we're not collecting at all (which is the whole point). With this refactoring, the mechanism (hijacking getLeafCollector) is moved into AggregatorBase. Aggregators that have a strategy to precompute their answer can override tryPrecomputeAggregationForLeaf, which is expected to return true if they managed to precompute. This should also make it easier to keep track of which aggregations have precomputation approaches (since they override this method). --------- (cherry picked from commit 2847695) Signed-off-by: Michael Froh <froh@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

sandeshkr419 · 2025-01-30T21:11:59Z

@msfroh Since this change is not a feature update, should we create a backport 2.19 as well?

One major advantage to backport in 2.19 I see is that any critical bugs if we have to backport to 2.19 in future, can be easily backported to 2.19 without having to worry about making too many manual changes. Thoughts?

cc - @rishabh6788 (2.19 Release Manager)

msfroh · 2025-01-30T21:53:37Z

@msfroh Since this change is not a feature update, should we create a backport 2.19 as well?

One major advantage to backport in 2.19 I see is that any critical bugs if we have to backport to 2.19 in future, can be easily backported to 2.19 without having to worry about making too many manual changes. Thoughts?

cc - @rishabh6788 (2.19 Release Manager)

That's a good question. Part of me says, "Well, I missed the 2.19 cut-off, so too bad". On the other hand, your argument about avoiding merge conflicts is also relevant. I'll defer to @rishabh6788's judgement.

@bowenlan-amzn

* Unify precomputation of aggregations behind a common API We've had a series of aggregation speedups that use the same strategy: instead of iterating through documents that match the query one-by-one, we can look at a Lucene segment and compute the aggregation directly (if some particular conditions are met). In every case, we've hooked that into custom logic hijacks the getLeafCollector method and throws CollectionTerminatedException. This creates the illusion that we're implementing a custom LeafCollector, when really we're not collecting at all (which is the whole point). With this refactoring, the mechanism (hijacking getLeafCollector) is moved into AggregatorBase. Aggregators that have a strategy to precompute their answer can override tryPrecomputeAggregationForLeaf, which is expected to return true if they managed to precompute. This should also make it easier to keep track of which aggregations have precomputation approaches (since they override this method). Signed-off-by: Michael Froh <froh@amazon.com> * Remove subaggregator check from CompositeAggregator Not sure why I added this, when the existing implementation didn't have it. That said, we *should* call finishLeaf() before precomputing the current leaf. Signed-off-by: Michael Froh <froh@amazon.com> * Resolve conflicts with star-tree changes Signed-off-by: Michael Froh <froh@amazon.com> * Skip precomputation when valuesSource is null Signed-off-by: Michael Froh <froh@amazon.com> * Add comment as suggested by @bowenlan-amzn Signed-off-by: Michael Froh <froh@amazon.com> --------- Signed-off-by: Michael Froh <froh@amazon.com> (cherry picked from commit 2847695) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

sandeshkr419 · 2025-01-30T23:08:56Z

Discussed with @rishabh6788 offline. We are in consensus to include this for the fore-mentioned reason. Adding up backport 2.19 label for the bot to create a backport PR.

…7212)

msfroh added the skip-changelog label Nov 27, 2024

opensearch-ci-bot mentioned this pull request Nov 28, 2024

[AUTOCUT] Gradle Check Flaky Test Report for RemoteRestoreSnapshotIT #14324

Open

bharath-techie reviewed Dec 3, 2024

View reviewed changes

server/src/main/java/org/opensearch/search/aggregations/AggregatorBase.java Show resolved Hide resolved

getsaurabh02 assigned msfroh Dec 9, 2024

msfroh marked this pull request as ready for review December 12, 2024 19:49

msfroh requested review from Bukhtawar, CEHENKLE, Rishikesh1159, VachaShah, anasalkouz, andrross, ashking94, dblock, dbwiddis, gbbafna, jed326, kotwanikunal, mch2, nknize, owaiskazi19, reta, sachinpkale, saratvemulapalli, shwetathareja and sohami as code owners December 12, 2024 19:49

msfroh force-pushed the agg_precomputation_API branch from c3897a0 to 19a40cc Compare January 29, 2025 20:06

sandeshkr419 reviewed Jan 29, 2025

View reviewed changes

server/src/main/java/org/opensearch/search/aggregations/metrics/MaxAggregator.java Show resolved Hide resolved

expani reviewed Jan 29, 2025

View reviewed changes

server/src/main/java/org/opensearch/search/aggregations/metrics/MinAggregator.java Show resolved Hide resolved

Skip precomputation when valuesSource is null

caceb62

Signed-off-by: Michael Froh <froh@amazon.com>

msfroh force-pushed the agg_precomputation_API branch from 4ac8bcb to caceb62 Compare January 29, 2025 23:13

sandeshkr419 approved these changes Jan 29, 2025

View reviewed changes

Add comment as suggested by @bowenlan-amzn

1c3c990

Signed-off-by: Michael Froh <froh@amazon.com>

jainankitk approved these changes Jan 30, 2025

View reviewed changes

jainankitk merged commit 2847695 into opensearch-project:main Jan 30, 2025
30 checks passed

jainankitk added the backport 2.x Backport to 2.x branch label Jan 30, 2025

opensearch-trigger-bot bot mentioned this pull request Jan 30, 2025

[Backport 2.x] Unify precomputation of aggregations behind a common API #17197

Merged

sandeshkr419 mentioned this pull request Jan 30, 2025

[Star Tree] [Search] Keyword & Numeric Terms Aggregation #17165

Merged

3 tasks

sandeshkr419 added the backport 2.19 label Jan 30, 2025

opensearch-trigger-bot bot mentioned this pull request Jan 30, 2025

[Backport 2.19] Unify precomputation of aggregations behind a common API #17212

Merged

sandeshkr419 added the v2.19.0 Issues and PRs related to version 2.19.0 label Jan 30, 2025

mch2 pushed a commit that referenced this pull request Jan 31, 2025

Unify precomputation of aggregations behind a common API (#16733) (#1…

b006c0f

…7212)

sandeshkr419 mentioned this pull request Mar 4, 2025

[bug fix] fix precompute ordering in max & min aggregator #17505

Merged

3 tasks

bruce-hong-glean mentioned this pull request Apr 17, 2025

[BUG] reduced performance on GlobalOrdinalsStringTermsAggregator #17979

Closed

tkykenmt mentioned this pull request Jan 13, 2026

[feature] Dependency Updates tkykenmt/opensearch-feature-explorer#2038

Closed

3 tasks

Conversation

msfroh commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

github-actions bot commented Nov 27, 2024

Uh oh!

Uh oh!

sandeshkr419 commented Dec 3, 2024

Uh oh!

github-actions bot commented Dec 12, 2024

Uh oh!

codecov bot commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

msfroh commented Jan 10, 2025

Uh oh!

msfroh commented Jan 29, 2025

Uh oh!

github-actions bot commented Jan 29, 2025

Uh oh!

sandeshkr419 commented Jan 29, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 30, 2025

Uh oh!

github-actions bot commented Jan 30, 2025

Uh oh!

Uh oh!

sandeshkr419 commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msfroh commented Jan 30, 2025

Uh oh!

sandeshkr419 commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

msfroh commented Nov 27, 2024 •

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

sandeshkr419 commented Jan 30, 2025 •

edited

Loading

sandeshkr419 commented Jan 30, 2025 •

edited

Loading