[Feature Request] Add a cluster setting for memory threshold to pick OrdinalsCollector in Cardinality aggregation #15269

rishabhmaurya · 2024-08-15T19:11:21Z

Is your feature request related to a problem? Please describe

Currently the logic to pick Ordinals vs DirectCollector is dynamic and based on the number of ordinals to collect and memory overhead. If memory overhead is high for OrdinalsCollector, then DirectCollector is used. Due to https://issues.apache.org/jira/browse/LUCENE-9663 few users are reporting regression in Cardinality aggregation because of slower DirectCollector after replacing prefix compression with LZ4 compression for terms dictionary in lucene 8.9.
Fix proposed here is to use OrdinalsCollector more often which will collect the ordinals into a bitset first and then performs term lookup in postCollect() of segment and that's a lot faster.
However, we don't have a way to control picking up OrdinalsCollector in OpenSearch.

Describe the solution you'd like

Introduce a memory threshold dynamic setting which OrdinalsCollector can use and if its usage is under this threshold, always use OrdinalsCollector. This logic can be added here.

Related component

Search:Aggregations

Describe alternatives you've considered

Use of eager_global_ordinals or murmur hash, but its an index time setting and can have impact on indexing performance. Its discussed in more detail here.

Additional context

No response

The text was updated successfully, but these errors were encountered:

mch2 · 2024-08-21T16:09:43Z

Thanks @rishabhmaurya seems like something we should do, am removing untriaged.

maitreya2954 · 2024-08-28T20:56:07Z

@mch2 I would like to work on this. I am new to contributing to OpenSearch. So, would appreciate any guidance.

jainankitk · 2024-08-28T20:58:51Z

Thanks @maitreya2954 for volunteering, assigned it to you!

rishabhmaurya · 2024-08-28T21:40:47Z

@maitreya2954 Appreciate you picking this up. Here is what we can do -

Introduce execution_hint for cardinality aggregation similar to term aggregation, with values: direct, ordinal to start with.
The default behaviour should be current behaviour. When execution_hint is provided, override the collector to be picked here and don't honour this condition here.
Add unit tests for all 3 cases.
Add documentation here by opening a documentation issue on this repo.

If you're blocked don't hesitate to ask for pointers. Thank you.

maitreya2954 · 2024-09-09T14:56:03Z

@rishabhmaurya I added execution hint field to the cardinality agg. However, I am stuck at adding unit tests.

Add unit tests for all 3 cases.

3 cases being: direct, ordinal and when no value is provided. right?

Here's my question: What type of query should I test after setting the execution hint. I can see many different type of queries being tested here. Should I just do MatchAllDocsQuery?

rishabhmaurya · 2024-09-09T16:15:11Z

@maitreya2954 That's great that you were able to add execution hint.

3 cases being: direct, ordinal and when no value is provided. right?

yes.

What type of query should I test after setting the execution hint. I can see many different type of queries being tested here. Should I just do MatchAllDocsQuery?

You can test for both match_all query and a filter using term query on a field other than cardinality aggregation field.

rishabhmaurya · 2024-09-09T16:23:36Z

@maitreya2954 how were you thinking about checking which collector is picked from tests?

One way could add a method to this class to get the current collector picked for a given segment and add a logic here to store the name/class -

OpenSearch/test/framework/src/main/java/org/opensearch/search/aggregations/AggregatorTestCase.java

Line 1207 in 978d14e

    
           public LeafBucketCollector getLeafCollector(LeafReaderContext ctx) throws IOException {

maitreya2954 · 2024-09-13T20:13:03Z

@rishabhmaurya Here's the PR: #15764

Documentation PR: opensearch-project/documentation-website#8265

rishabhmaurya · 2024-09-13T20:49:52Z

@maitreya2954 great, thanks! I will take a look shortly. Meanwhile you can work on fixing gradle checks which are failing.

getsaurabh02 · 2024-10-02T16:37:29Z

Thanks @maitreya2954 for raising the PR. Is it something we can target for 2.18?
cc: @rishabhmaurya

maitreya2954 · 2024-10-03T18:26:09Z

@rishabhmaurya From the conversation in the PR, I am assuming we are going to add the cluster setting as well for the memory threshold. Can you guide me to a previous PR that added a cluster setting for reference.

@getsaurabh02 I am not sure, how long the new changes might take. @rishabhmaurya can we target for 2.18.

sandeshkr419 · 2024-12-04T17:24:38Z

@maitreya2954 Are you still working on it?

maitreya2954 · 2024-12-16T15:50:55Z

@sandeshkr419 Yes, I will start working was busy with something else. Since, I am very new to this project, can you guide me to a previous PR that added a cluster setting. I will refer that and make changes. Thank you

asimmahmood1 · 2025-02-17T19:50:53Z

Plan to follow up to #17312

asimmahmood1 · 2025-02-20T19:45:29Z

So compared 3 different cardinality runs. Current osb cardinality-agg-high uses agent.name and there is little difference with default vs ordinals only. event.id has almost 7x cardinality and where ordinals is must faster. So I think its worth adding this to osb, e.g. cardinality-agg-vhigh

osb task		cardinality-agg-high	cardinality-agg-low
field	event.id	agent.name	cloud.region
cardinality	180250	26832	25

took_time
no hint	79802	2342	10
ordinals	2982	2120	10
direct	79673	114579	10

Looking at profiler, agent.name uses all ordinals collector, so it makes sense it doesn't make a difference.

asimmahmood1 · 2025-02-20T19:48:00Z

OSB result using latest main branch. This also shows litlte difference between default (baseline) and ordinals only (contender) result.

dev-dsk-asimmahm-2c-a6d21262 % opensearch-benchmark compare --baseline=a1947387-a454-4348-ae5a-27260c7ed2e1 --contender=5e2a9ea4-4500-44a8-b215-fa245ad9a6f9

   ____                  _____                      __       ____                  __                         __
  / __ \____  ___  ____ / ___/___  ____ ___________/ /_     / __ )___  ____  _____/ /_  ____ ___  ____ ______/ /__
 / / / / __ \/ _ \/ __ \\__ \/ _ \/ __ `/ ___/ ___/ __ \   / __  / _ \/ __ \/ ___/ __ \/ __ `__ \/ __ `/ ___/ //_/
/ /_/ / /_/ /  __/ / / /__/ /  __/ /_/ / /  / /__/ / / /  / /_/ /  __/ / / / /__/ / / / / / / / / /_/ / /  / ,<
\____/ .___/\___/_/ /_/____/\___/\__,_/_/   \___/_/ /_/  /_____/\___/_/ /_/\___/_/ /_/_/ /_/ /_/\__,_/_/  /_/|_|
    /_/


Comparing baseline
  TestExecution ID: a1947387-a454-4348-ae5a-27260c7ed2e1
  TestExecution timestamp: 2025-02-20 19:36:06
  TestProcedure: big5
  ProvisionConfigInstance: external

with contender
  TestExecution ID: 5e2a9ea4-4500-44a8-b215-fa245ad9a6f9
  TestExecution timestamp: 2025-02-20 19:22:13
  TestProcedure: big5
  ProvisionConfigInstance: external

------------------------------------------------------
    _______             __   _____
   / ____(_)___  ____ _/ /  / ___/_________  ________
  / /_  / / __ \/ __ `/ /   \__ \/ ___/ __ \/ ___/ _ \
 / __/ / / / / / /_/ / /   ___/ / /__/ /_/ / /  /  __/
/_/   /_/_/ /_/\__,_/_/   /____/\___/\____/_/   \___/
------------------------------------------------------

|                                                        Metric |                 Task |    Baseline |   Contender |     Diff |   Unit |
|--------------------------------------------------------------:|---------------------:|------------:|------------:|---------:|-------:|
|                    Cumulative indexing time of primary shards |                      |           0 |           0 |        0 |    min |
|             Min cumulative indexing time across primary shard |                      |           0 |           0 |        0 |    min |
|          Median cumulative indexing time across primary shard |                      |           0 |           0 |        0 |    min |
|             Max cumulative indexing time across primary shard |                      |           0 |           0 |        0 |    min |
|           Cumulative indexing throttle time of primary shards |                      |           0 |           0 |        0 |    min |
|    Min cumulative indexing throttle time across primary shard |                      |           0 |           0 |        0 |    min |
| Median cumulative indexing throttle time across primary shard |                      |           0 |           0 |        0 |    min |
|    Max cumulative indexing throttle time across primary shard |                      |           0 |           0 |        0 |    min |
|                       Cumulative merge time of primary shards |                      |           0 |           0 |        0 |    min |
|                      Cumulative merge count of primary shards |                      |           0 |           0 |        0 |        |
|                Min cumulative merge time across primary shard |                      |           0 |           0 |        0 |    min |
|             Median cumulative merge time across primary shard |                      |           0 |           0 |        0 |    min |
|                Max cumulative merge time across primary shard |                      |           0 |           0 |        0 |    min |
|              Cumulative merge throttle time of primary shards |                      |           0 |           0 |        0 |    min |
|       Min cumulative merge throttle time across primary shard |                      |           0 |           0 |        0 |    min |
|    Median cumulative merge throttle time across primary shard |                      |           0 |           0 |        0 |    min |
|       Max cumulative merge throttle time across primary shard |                      |           0 |           0 |        0 |    min |
|                     Cumulative refresh time of primary shards |                      |           0 |           0 |        0 |    min |
|                    Cumulative refresh count of primary shards |                      |           2 |           2 |        0 |        |
|              Min cumulative refresh time across primary shard |                      |           0 |           0 |        0 |    min |
|           Median cumulative refresh time across primary shard |                      |           0 |           0 |        0 |    min |
|              Max cumulative refresh time across primary shard |                      |           0 |           0 |        0 |    min |
|                       Cumulative flush time of primary shards |                      |           0 |           0 |        0 |    min |
|                      Cumulative flush count of primary shards |                      |           1 |           1 |        0 |        |
|                Min cumulative flush time across primary shard |                      |           0 |           0 |        0 |    min |
|             Median cumulative flush time across primary shard |                      |           0 |           0 |        0 |    min |
|                Max cumulative flush time across primary shard |                      |           0 |           0 |        0 |    min |
|                                       Total Young Gen GC time |                      |           0 |           0 |        0 |      s |
|                                      Total Young Gen GC count |                      |           0 |           0 |        0 |        |
|                                         Total Old Gen GC time |                      |           0 |           0 |        0 |      s |
|                                        Total Old Gen GC count |                      |           0 |           0 |        0 |        |
|                                                    Store size |                      |     25.6365 |     25.6365 |        0 |     GB |
|                                                 Translog size |                      | 5.12227e-08 | 5.12227e-08 |        0 |     GB |
|                                        Heap used for segments |                      |           0 |           0 |        0 |     MB |
|                                      Heap used for doc values |                      |           0 |           0 |        0 |     MB |
|                                           Heap used for terms |                      |           0 |           0 |        0 |     MB |
|                                           Heap used for norms |                      |           0 |           0 |        0 |     MB |
|                                          Heap used for points |                      |           0 |           0 |        0 |     MB |
|                                   Heap used for stored fields |                      |           0 |           0 |        0 |     MB |
|                                                 Segment count |                      |          15 |          15 |        0 |        |
|                                                Min Throughput | cardinality-agg-high |    0.734983 |    0.737579 |   0.0026 |  ops/s |
|                                               Mean Throughput | cardinality-agg-high |    0.735557 |    0.737762 |  0.00221 |  ops/s |
|                                             Median Throughput | cardinality-agg-high |    0.735597 |    0.737758 |  0.00216 |  ops/s |
|                                                Max Throughput | cardinality-agg-high |    0.735983 |     0.73807 |  0.00209 |  ops/s |
|                                       50th percentile latency | cardinality-agg-high |      215606 |      214677 |   -929.1 |     ms |
|                                       90th percentile latency | cardinality-agg-high |      249461 |      248494 | -966.712 |     ms |
|                                       99th percentile latency | cardinality-agg-high |      257099 |      256041 | -1057.65 |     ms |
|                                      100th percentile latency | cardinality-agg-high |      257950 |      256875 | -1074.98 |     ms |
|                                  50th percentile service time | cardinality-agg-high |     1349.15 |     1352.07 |  2.92024 |     ms |
|                                  90th percentile service time | cardinality-agg-high |     1373.48 |     1370.18 | -3.30218 |     ms |
|                                  99th percentile service time | cardinality-agg-high |     1415.66 |     1406.76 | -8.90239 |     ms |
|                                 100th percentile service time | cardinality-agg-high |     1416.66 |      1427.1 |  10.4418 |     ms |
|                                                    error rate | cardinality-agg-high |           0 |           0 |        0 |      % |


-------------------------------
[INFO] SUCCESS (took 0 seconds)
-------------------------------

getsaurabh02 · 2025-02-26T17:40:09Z

@asimmahmood1 Do we still need this? Given we have a request level hint now, can we close this?

rishabhmaurya added enhancement Enhancement or improvement to existing feature or request good first issue Good for newcomers untriaged labels Aug 15, 2024

rishabhmaurya added this to Search Project Board Aug 15, 2024

github-project-automation bot moved this to 🆕 New in Search Project Board Aug 15, 2024

github-actions bot added the Search:Aggregations label Aug 15, 2024

mch2 removed the untriaged label Aug 21, 2024

jainankitk assigned maitreya2954 Aug 28, 2024

maitreya2954 linked a pull request Sep 5, 2024 that will close this issue

Introduce execution hint for Cardinality aggregation #15764

Open

3 tasks

This was referenced Feb 3, 2025

[Feature Request] Use of Binary DocValue for high cardinality fields to improve aggregations performance #16837

Closed

Introduce execution hint for Cardinality aggregation #17301

Closed

Introduce execution_hint for Cardinality aggregation #17312

Merged

getsaurabh02 added this to Performance Roadmap Feb 17, 2025

github-project-automation bot moved this to Todo in Performance Roadmap Feb 17, 2025

bowenlan-amzn assigned asimmahmood1 Feb 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature Request] Add a cluster setting for memory threshold to pick OrdinalsCollector in Cardinality aggregation #15269

[Feature Request] Add a cluster setting for memory threshold to pick OrdinalsCollector in Cardinality aggregation #15269

rishabhmaurya commented Aug 15, 2024 •

edited

Loading

mch2 commented Aug 21, 2024

maitreya2954 commented Aug 28, 2024 •

edited

Loading

jainankitk commented Aug 28, 2024

rishabhmaurya commented Aug 28, 2024

maitreya2954 commented Sep 9, 2024

rishabhmaurya commented Sep 9, 2024 •

edited

Loading

rishabhmaurya commented Sep 9, 2024

maitreya2954 commented Sep 13, 2024 •

edited

Loading

rishabhmaurya commented Sep 13, 2024

getsaurabh02 commented Oct 2, 2024

maitreya2954 commented Oct 3, 2024

sandeshkr419 commented Dec 4, 2024

maitreya2954 commented Dec 16, 2024

asimmahmood1 commented Feb 17, 2025

asimmahmood1 commented Feb 20, 2025 •

edited

Loading

asimmahmood1 commented Feb 20, 2025

getsaurabh02 commented Feb 26, 2025

[Feature Request] Add a cluster setting for memory threshold to pick OrdinalsCollector in Cardinality aggregation #15269

[Feature Request] Add a cluster setting for memory threshold to pick OrdinalsCollector in Cardinality aggregation #15269

Comments

rishabhmaurya commented Aug 15, 2024 • edited Loading

Is your feature request related to a problem? Please describe

Describe the solution you'd like

Related component

Describe alternatives you've considered

Additional context

mch2 commented Aug 21, 2024

maitreya2954 commented Aug 28, 2024 • edited Loading

jainankitk commented Aug 28, 2024

rishabhmaurya commented Aug 28, 2024

maitreya2954 commented Sep 9, 2024

rishabhmaurya commented Sep 9, 2024 • edited Loading

rishabhmaurya commented Sep 9, 2024

maitreya2954 commented Sep 13, 2024 • edited Loading

rishabhmaurya commented Sep 13, 2024

getsaurabh02 commented Oct 2, 2024

maitreya2954 commented Oct 3, 2024

sandeshkr419 commented Dec 4, 2024

maitreya2954 commented Dec 16, 2024

asimmahmood1 commented Feb 17, 2025

asimmahmood1 commented Feb 20, 2025 • edited Loading

asimmahmood1 commented Feb 20, 2025

getsaurabh02 commented Feb 26, 2025

rishabhmaurya commented Aug 15, 2024 •

edited

Loading

maitreya2954 commented Aug 28, 2024 •

edited

Loading

rishabhmaurya commented Sep 9, 2024 •

edited

Loading

maitreya2954 commented Sep 13, 2024 •

edited

Loading

asimmahmood1 commented Feb 20, 2025 •

edited

Loading