[ESQL] Adds HeapAttack tests for LIMIT BY and SORT | LIMIT BY#145115
Merged
ncordon merged 23 commits intoelastic:mainfrom Mar 31, 2026
Merged
[ESQL] Adds HeapAttack tests for LIMIT BY and SORT | LIMIT BY#145115ncordon merged 23 commits intoelastic:mainfrom
ncordon merged 23 commits intoelastic:mainfrom
Conversation
418e5eb to
a25e1f5
Compare
Collaborator
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
ncordon
commented
Mar 30, 2026
...heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java
Show resolved
Hide resolved
ncordon
commented
Mar 30, 2026
...heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java
Show resolved
Hide resolved
ncordon
commented
Mar 30, 2026
...heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java
Show resolved
Hide resolved
ncordon
commented
Mar 30, 2026
ncordon
commented
Mar 30, 2026
...ugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/GroupedLimitOperator.java
Outdated
Show resolved
Hide resolved
ab709b2 to
91a4b1d
Compare
ncordon
commented
Mar 30, 2026
...ttack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackTestCase.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Adds new ESQL heap-attack REST tests focused on circuit-breaking behavior in LIMIT BY and SORT | LIMIT BY operators, and factors shared heap-attack helpers into HeapAttackTestCase to support these tests.
Changes:
- Add
HeapAttackLimitByITcovering multiple circuit-breaking paths inGroupedLimitOperatorandGroupedTopNOperator. - Add
assertCircuitBreaksVia(...)plus shared dataset/query helpers toHeapAttackTestCase(and remove duplicates fromHeapAttackIT). - Adjust compute/operator code to support the new tests (resource cleanup + class visibility) and add test module dependencies.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/TopNRow.java | Makes TopNRow public so heap-attack tests can reference it for stack-trace assertions. |
| x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/TopNQueue.java | Makes TopNQueue public so heap-attack tests can reference it for stack-trace assertions. |
| x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/GroupedLimitOperator.java | Ensures counts is released on constructor failure. |
| test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackTestCase.java | Adds assertCircuitBreaksVia(...) and shared index/query helpers (initManyLongs, makeManyLongs, etc.). |
| test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackLimitByIT.java | New integration tests for heap-attack coverage of LIMIT BY and SORT | LIMIT BY. |
| test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java | Removes duplicated helper methods now hosted in HeapAttackTestCase. |
| test/external-modules/esql-heap-attack/build.gradle | Adds test dependencies on :x-pack:plugin:esql:compute and :libs:swisshash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/TopNRow.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/TopNQueue.java
Outdated
Show resolved
Hide resolved
...ttack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackTestCase.java
Show resolved
Hide resolved
...ttack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackTestCase.java
Show resolved
Hide resolved
...tack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackLimitByIT.java
Outdated
Show resolved
Hide resolved
nik9000
reviewed
Mar 30, 2026
...tack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackLimitByIT.java
Show resolved
Hide resolved
...ttack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackTestCase.java
Outdated
Show resolved
Hide resolved
...tack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackLimitByIT.java
Show resolved
Hide resolved
julian-elastic
approved these changes
Mar 30, 2026
Contributor
julian-elastic
left a comment
There was a problem hiding this comment.
Overall looks good, should be good to merge after the comments are addressed!
This reverts commit eb1d71a.
ncordon
added a commit
to ncordon/elasticsearch
that referenced
this pull request
Apr 1, 2026
mromaios
pushed a commit
to mromaios/elasticsearch
that referenced
this pull request
Apr 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds HeapAttackLimitByIT, covering circuit-breaking inside GroupedLimitOperator and GroupedTopNOperator. Shared helpers are moved to HeapAttackTestCase.
Paths tested:
seenKeyshash table (wide group keys),GroupKeyEncoderscratch buffer (many large strings), large group limit (N in LIMIT N BY)TopNRowstorage (many sort columns),keysHash(wide group keys),TopNQueuepre-allocation (many groups × large per-group limit) andGroupKeyEncoderscratch buffer (many large strings), large group limit (N in LIMIT N BY)Path not tested:
countsarray in GroupedLimitOperator: only 4 bytes/group, so seenKeys always trips first.Also adds
assertCircuitBreaksVia(TryCircuitBreaking, String... classNames)which validates the exception stack trace contains the expected classes, and retries if a different part of the pipeline trips first.Addresses https://github.com/elastic/esql-planning/issues/404