Avoid O(N^2) in VALUES with ordinals grouping #130576

dnhatn · 2025-07-03T19:28:08Z

Using the VALUES aggregator with ordinals grouping led to accidental quadratic complexity. Queries like FROM .. | STATS ... VALUES(field) ... BY keyword-field are affected by this performance issue. This change caches a sorted structure - previously used to fix a similar O(N^2) problem when emitting the output block - during the merging phase of the OrdinalGroupingOperator.

elasticsearchmachine · 2025-07-04T04:01:04Z

Hi @dnhatn, I've created a changelog YAML for you.

dnhatn · 2025-07-04T04:01:17Z

I updated the benchmark to simulate the ordinal grouping operator. With the main branch, I could not complete the benchmark with 1,000,000 groups, but with the fix, it took 1625ms. The benchmark results are below.

Before:
Benchmark                      (dataType)  (groups)  (numOrdinalMerges)  Mode  Cnt      Score   Error  Units
ValuesAggregatorBenchmark.run    BytesRef         1                   0  avgt    2      3.680          ms/op
ValuesAggregatorBenchmark.run    BytesRef         1                   1  avgt    2      3.632          ms/op
ValuesAggregatorBenchmark.run    BytesRef      1000                   0  avgt    2      2.515          ms/op
ValuesAggregatorBenchmark.run    BytesRef      1000                   1  avgt    2      9.397          ms/op
ValuesAggregatorBenchmark.run    BytesRef    200000                   0  avgt    2    148.966          ms/op
ValuesAggregatorBenchmark.run    BytesRef    200000                   1  avgt    2  90055.908          ms/op
ValuesAggregatorBenchmark.run         int         1                   0  avgt    2      0.494          ms/op
ValuesAggregatorBenchmark.run         int         1                   1  avgt    2      0.488          ms/op
ValuesAggregatorBenchmark.run         int      1000                   0  avgt    2      2.788          ms/op
ValuesAggregatorBenchmark.run         int      1000                   1  avgt    2      8.232          ms/op
ValuesAggregatorBenchmark.run         int    200000                   0  avgt    2    198.020          ms/op
ValuesAggregatorBenchmark.run         int    200000                   1  avgt    2  70918.020          ms/op
ValuesAggregatorBenchmark.run        long         1                   0  avgt    2      0.862          ms/op
ValuesAggregatorBenchmark.run        long         1                   1  avgt    2      0.873          ms/op
ValuesAggregatorBenchmark.run        long      1000                   0  avgt    2      4.212          ms/op
ValuesAggregatorBenchmark.run        long      1000                   1  avgt    2     10.450          ms/op
ValuesAggregatorBenchmark.run        long    200000                   0  avgt    2    257.926          ms/op
ValuesAggregatorBenchmark.run        long    200000                   1  avgt    2  75686.076          ms/op



After:
Benchmark                      (dataType)  (groups)  (numOrdinalMerges)  Mode  Cnt     Score   Error  Units
ValuesAggregatorBenchmark.run    BytesRef         1                   0  avgt    2     3.909          ms/op
ValuesAggregatorBenchmark.run    BytesRef         1                   1  avgt    2     3.951          ms/op
ValuesAggregatorBenchmark.run    BytesRef      1000                   0  avgt    2     2.635          ms/op
ValuesAggregatorBenchmark.run    BytesRef      1000                   1  avgt    2     2.703          ms/op
ValuesAggregatorBenchmark.run    BytesRef   1000000                   0  avgt    2  1519.385          ms/op
ValuesAggregatorBenchmark.run    BytesRef   1000000                   1  avgt    2  1623.915          ms/op
ValuesAggregatorBenchmark.run         int         1                   0  avgt    2     0.601          ms/op
ValuesAggregatorBenchmark.run         int         1                   1  avgt    2     0.613          ms/op
ValuesAggregatorBenchmark.run         int      1000                   0  avgt    2     2.504          ms/op
ValuesAggregatorBenchmark.run         int      1000                   1  avgt    2     2.591          ms/op
ValuesAggregatorBenchmark.run         int   1000000                   0  avgt    2  1396.017          ms/op
ValuesAggregatorBenchmark.run         int   1000000                   1  avgt    2  1441.373          ms/op
ValuesAggregatorBenchmark.run        long         1                   0  avgt    2     0.598          ms/op
ValuesAggregatorBenchmark.run        long         1                   1  avgt    2     0.597          ms/op
ValuesAggregatorBenchmark.run        long      1000                   0  avgt    2     2.397          ms/op
ValuesAggregatorBenchmark.run        long      1000                   1  avgt    2     2.510          ms/op
ValuesAggregatorBenchmark.run        long   1000000                   0  avgt    2  1538.923          ms/op
ValuesAggregatorBenchmark.run        long   1000000                   1  avgt    2  1625.971          ms/op

elasticsearchmachine · 2025-07-04T04:02:31Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

nik9000 · 2025-07-07T12:08:07Z

...ks/src/main/java/org/elasticsearch/benchmark/compute/operator/ValuesAggregatorBenchmark.java

@@ -65,7 +65,7 @@
 @Fork(1)
 public class ValuesAggregatorBenchmark {
    static final int MIN_BLOCK_LENGTH = 8 * 1024;
-    private static final int OP_COUNT = 1024;
+    private static final int OP_COUNT = 20;


Is this a temporary change or permanent?

Ah, sorry, I forgot to add a comment for this. Did we intentionally set the HashOperator to use 1000 pages? Would 100 or 50 pages be enough instead? I have reverted this change; let's discuss it in a separate PR.

nik9000 · 2025-07-07T12:25:30Z

...e/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregator.java

+                        // no longer need the bytes
+                        bytes.close();
+                        bytes = null;
+                        int[] ids = sortedForOrdinalMerging.ids;


Could you remove this line? I spent like ten minutes staring at this code to try and figure out why it exists. But I was missing this line. Without it the loop's super obviously changing values in the ids array.

Could you add a @param to the ids field of Sorted to say something like this is the position in my array of the values to read OR, if build from {@link buildForMerging} then it's the position into the *target* state to merge into or something like that. I haven't had enough coffee to make those words make sense. But it's interesting that this means different things depending on where you got it.

++ I've added a javadoc in 6401294

nik9000 · 2025-07-07T12:31:02Z

.../esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st

                    long both = values.get(id);
                    int group = (int) (both >>> Float.SIZE);
-$endif$
+        $endif$


Is the indent better? I liked having them on the left so they were easier to see as controls.

Copy-pasted issue. I have reverted this.

nik9000 · 2025-07-07T12:32:00Z

.../esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st

+        public void close() {
+            releasable.close();
+        }
+    }


I wonder if this should be it's own little java class rather than generated once per.

nik9000

Actually want to block merging on my question about OP_COUNT - once we get that settled it's cool.

Using the VALUES aggregator with ordinals grouping led to accidental quadratic complexity. Queries like FROM .. | STATS ... VALUES(field) ... BY keyword-field are affected by this performance issue. This change caches a sorted structure - previously used to fix a similar O(N^2) problem when emitting the output block - during the merging phase of the OrdinalGroupingOperator.

Using the VALUES aggregator with ordinals grouping led to accidental quadratic complexity. Queries like FROM .. | STATS ... VALUES(field) ... BY keyword-field are affected by this performance issue. This change caches a sorted structure - previously used to fix a similar O(N^2) problem when emitting the output block - during the merging phase of the OrdinalGroupingOperator. # Conflicts: # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesDoubleAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesFloatAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesIntAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesLongAggregator.java # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregators.java # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st

Using the VALUES aggregator with ordinals grouping led to accidental quadratic complexity. Queries like FROM .. | STATS ... VALUES(field) ... BY keyword-field are affected by this performance issue. This change caches a sorted structure - previously used to fix a similar O(N^2) problem when emitting the output block - during the merging phase of the OrdinalGroupingOperator. (cherry picked from commit 59df1bf) # Conflicts: # benchmarks/src/main/java/org/elasticsearch/benchmark/compute/operator/ValuesAggregatorBenchmark.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesDoubleAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesFloatAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesIntAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesLongAggregator.java # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregators.java # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st

dnhatn · 2025-07-07T23:58:45Z

💚 All backports created successfully

Status	Branch	Result
✅	9.0
✅	8.19
✅	8.18
✅	8.17

Questions ?

Please refer to the Backport tool documentation

Using the VALUES aggregator with ordinals grouping led to accidental quadratic complexity. Queries like FROM .. | STATS ... VALUES(field) ... BY keyword-field are affected by this performance issue. This change caches a sorted structure - previously used to fix a similar O(N^2) problem when emitting the output block - during the merging phase of the OrdinalGroupingOperator. (cherry picked from commit 59df1bf) # Conflicts: # benchmarks/src/main/java/org/elasticsearch/benchmark/compute/operator/ValuesAggregatorBenchmark.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesDoubleAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesFloatAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesIntAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesLongAggregator.java # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregators.java # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st # Conflicts: # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregator.java # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st

Using the VALUES aggregator with ordinals grouping led to accidental quadratic complexity. Queries like FROM .. | STATS ... VALUES(field) ... BY keyword-field are affected by this performance issue. This change caches a sorted structure - previously used to fix a similar O(N^2) problem when emitting the output block - during the merging phase of the OrdinalGroupingOperator. # Conflicts: # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesDoubleAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesFloatAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesIntAggregator.java # x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesLongAggregator.java # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregators.java # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st

Using the VALUES aggregator with ordinals grouping led to accidental quadratic complexity. Queries like FROM .. | STATS ... VALUES(field) ... BY keyword-field are affected by this performance issue. This change caches a sorted structure - previously used to fix a similar O(N^2) problem when emitting the output block - during the merging phase of the OrdinalGroupingOperator.

We should not build the sorted structure for the ordinal grouping operator if the requested position is larger than maxGroupId. This situation occurs with nulls. We should benchmark the ordinal blocks and consider removing the ordinal grouping operator if performance is similar; otherwise, we need to integrate this operator with GroupingAggregatorFunctionTestCase. Relates #130576

We should not build the sorted structure for the ordinal grouping operator if the requested position is larger than maxGroupId. This situation occurs with nulls. We should benchmark the ordinal blocks and consider removing the ordinal grouping operator if performance is similar; otherwise, we need to integrate this operator with GroupingAggregatorFunctionTestCase. Relates elastic#130576

We should not build the sorted structure for the ordinal grouping operator if the requested position is larger than maxGroupId. This situation occurs with nulls. We should benchmark the ordinal blocks and consider removing the ordinal grouping operator if performance is similar; otherwise, we need to integrate this operator with GroupingAggregatorFunctionTestCase. Relates elastic#130576 (cherry picked from commit f58d291)

We should not build the sorted structure for the ordinal grouping operator if the requested position is larger than maxGroupId. This situation occurs with nulls. We should benchmark the ordinal blocks and consider removing the ordinal grouping operator if performance is similar; otherwise, we need to integrate this operator with GroupingAggregatorFunctionTestCase. Relates elastic#130576 (cherry picked from commit f58d291) # Conflicts: # x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/OperatorTests.java

We should not build the sorted structure for the ordinal grouping operator if the requested position is larger than maxGroupId. This situation occurs with nulls. We should benchmark the ordinal blocks and consider removing the ordinal grouping operator if performance is similar; otherwise, we need to integrate this operator with GroupingAggregatorFunctionTestCase. Relates #130576

* Fix empty VALUES with ordinals grouping (#130861) We should not build the sorted structure for the ordinal grouping operator if the requested position is larger than maxGroupId. This situation occurs with nulls. We should benchmark the ordinal blocks and consider removing the ordinal grouping operator if performance is similar; otherwise, we need to integrate this operator with GroupingAggregatorFunctionTestCase. Relates #130576 * Fix test

We should not build the sorted structure for the ordinal grouping operator if the requested position is larger than maxGroupId. This situation occurs with nulls. We should benchmark the ordinal blocks and consider removing the ordinal grouping operator if performance is similar; otherwise, we need to integrate this operator with GroupingAggregatorFunctionTestCase. Relates #130576 (cherry picked from commit f58d291) # Conflicts: # x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/OperatorTests.java

We should not build the sorted structure for the ordinal grouping operator if the requested position is larger than maxGroupId. This situation occurs with nulls. We should benchmark the ordinal blocks and consider removing the ordinal grouping operator if performance is similar; otherwise, we need to integrate this operator with GroupingAggregatorFunctionTestCase. Relates #130576 (cherry picked from commit f58d291)

The ordinals grouping operator was introduced to speed up aggregation before ordinal blocks and related optimizations in block hashes were available. However, this operator has several issues: 1. It only supports single grouping with the `keyword` type and requires `doc_values`. 2. It needs a separate aggregation implementation, which currently lacks test coverage. We had performance issues with the `VALUES` aggregation using this operator (see #130576). 3. It can be slower and use more memory when the target documents have sparse cardinality (see #98963). 4. Ad-hoc planning, although this can now be addressed with local plans. Although the ordinals grouping operator is slightly faster than the hash operator with ordinal blocks, its complexity now outweighs the benefits. This PR proposes removing the operator. Below is the NYC_taxis benchmark. Closes #98963

elasticsearchmachine added the v9.2.0 label Jul 3, 2025

dnhatn force-pushed the fix-values-aggregator branch from 73349d4 to ef7d00c Compare July 3, 2025 22:29

dnhatn closed this Jul 3, 2025

dnhatn deleted the fix-values-aggregator branch July 3, 2025 22:29

dnhatn restored the fix-values-aggregator branch July 3, 2025 22:29

dnhatn reopened this Jul 3, 2025

dnhatn force-pushed the fix-values-aggregator branch from ef7d00c to 81326cb Compare July 3, 2025 22:39

Avoid O(N^2) in VALUES with OrdinalGrouping

0ee7d9e

dnhatn force-pushed the fix-values-aggregator branch from 81326cb to 0ee7d9e Compare July 4, 2025 03:49

[CI] Auto commit changes from spotless

814abc7

dnhatn added v9.1.1 v9.0.4 v8.19.1 v8.18.4 :Analytics/ES|QL AKA ESQL >bug labels Jul 4, 2025

Update docs/changelog/130576.yaml

4c44fa1

dnhatn requested a review from nik9000 July 4, 2025 04:01

dnhatn marked this pull request as ready for review July 4, 2025 04:02

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 4, 2025

Merge remote-tracking branch 'elastic/main' into fix-values-aggregator

c609cd1

nik9000 approved these changes Jul 7, 2025

View reviewed changes

nik9000 requested changes Jul 7, 2025

View reviewed changes

dnhatn added 4 commits July 7, 2025 07:59

revert

3e363b2

indent

d8e6b20

inline

f57c0c3

javadoc for ids

6401294

dnhatn mentioned this pull request Jul 7, 2025

[9.0] Avoid O(N^2) in VALUES with ordinals grouping (#130576) #130773

Merged

dnhatn mentioned this pull request Jul 7, 2025

[8.19] Avoid O(N^2) in VALUES with ordinals grouping (#130576) #130775

Merged

dnhatn mentioned this pull request Jul 7, 2025

[8.18] Avoid O(N^2) in VALUES with ordinals grouping (#130576) #130778

Merged

dnhatn mentioned this pull request Jul 7, 2025

[8.17] Avoid O(N^2) in VALUES with ordinals grouping (#130576) #130779

Merged

dnhatn mentioned this pull request Jul 8, 2025

Fix empty VALUES with ordinals grouping #130861

Merged

dnhatn removed the backport pending label Jul 8, 2025

dnhatn mentioned this pull request Jul 12, 2025

Remove ordinal grouping operator #131133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid O(N^2) in VALUES with ordinals grouping #130576

Avoid O(N^2) in VALUES with ordinals grouping #130576

Uh oh!

dnhatn commented Jul 3, 2025 •

edited

Loading

Uh oh!

elasticsearchmachine commented Jul 4, 2025

Uh oh!

dnhatn commented Jul 4, 2025

Uh oh!

elasticsearchmachine commented Jul 4, 2025

Uh oh!

nik9000 Jul 7, 2025

Uh oh!

dnhatn Jul 7, 2025

Uh oh!

nik9000 Jul 7, 2025

Uh oh!

nik9000 Jul 7, 2025

Uh oh!

dnhatn Jul 7, 2025

Uh oh!

nik9000 Jul 7, 2025

Uh oh!

dnhatn Jul 7, 2025

Uh oh!

nik9000 Jul 7, 2025

Uh oh!

nik9000 left a comment

Uh oh!

dnhatn commented Jul 7, 2025

Uh oh!

Uh oh!

Avoid O(N^2) in VALUES with ordinals grouping #130576

Avoid O(N^2) in VALUES with ordinals grouping #130576

Uh oh!

Conversation

dnhatn commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 4, 2025

Uh oh!

dnhatn commented Jul 4, 2025

Uh oh!

elasticsearchmachine commented Jul 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

dnhatn commented Jul 7, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Uh oh!

dnhatn commented Jul 3, 2025 •

edited

Loading