Skip to content

[ES|QL] Remove implicit limit appended to each subquery branch#139058

Merged
fang-xing-esql merged 41 commits intoelastic:mainfrom
fang-xing-esql:remove-implicit-limit-from-unionall-branches
Jan 14, 2026
Merged

[ES|QL] Remove implicit limit appended to each subquery branch#139058
fang-xing-esql merged 41 commits intoelastic:mainfrom
fang-xing-esql:remove-implicit-limit-from-unionall-branches

Conversation

@fang-xing-esql
Copy link
Member

@fang-xing-esql fang-xing-esql commented Dec 4, 2025

Resolves: #138106

The implicit limit appended to each subquery adds limitations for subquery, especially when working with inline stats. It may also cause non-deterministic results from subqueries when there is no sort in the subquery to ensure the order of the intermediate results.

This PR attempts to remove the implicit limit append to each subquery branch, and enables more capabilities of subquery. There is potential risk that the intermediate results returned by a subquery is huge, and they need to be processed in batches or CB if it is too big to process.

The major changes are listed here, there is NO change to Fork. More follow ups on heap attack tests are needed, this PR focus on functionality, making sure correct results are returned without the implicit limit appended to each subquery. The subquery feature is still behind snapshot.

  • HeapAttackSubqueryIT is added to catch potential OOM or CBE caused by large intermediate results from subqueries. Some tests with 8 subqueries hit OOM, instead of CBE, there are PR and new issues created to address those OOMs as follow ups. Currently most of the subquery heap attack tests run with 2 subqueries, root causes of the OOMs and TODOs and commented in each test.
  • PushDownFilterAndLimitIntoUnionAll is updated accordingly after Analyzer does not add the implicit limit to each subquery branch.
    • There is a special case related to the knn function. When a subquery contains a knn, it still requires a limit to appear after the knn, because knn has an implicitK that is not serialized or sent to the remote nodes. This was not an issue previously, as an implicit limit was appended to each branch. The PushDownFilterAndLimitIntoUnionAll rule has been updated to handle this knn case. Without a limit following the knn, the query either fails LogicalVerifier or produces incorrect results.
    • The sort operator has a similar situation, as unbounded sort is not supported. If a subquery contains a sort without a limit, removing the implicit limit previously appended to each branch causes LogicalVerifier to fail. In this PR, we do not append an implicit limit for this case, instead, it lets LogicalVerifier fail and inform users of the unbounded sort. If support for this query pattern is required in the future, an implicit limit could be appended, using an approach similar to the one applied for knn.

Some details on the OOM happened to the new subquery heap attack tests
There are some OOM discovered by the new subquery heap attack tests, they exposed some untracked memory used during reading(unsorted or sorted) data from lucene, the list of issues and PRs are below. These will be fixed as follow ups. This PR focus on functionality, instead of memory intensive queries.

@elasticsearchmachine
Copy link
Collaborator

Hi @fang-xing-esql, I've created a changelog YAML for you.

@fang-xing-esql fang-xing-esql added the test-release Trigger CI checks against release build label Dec 8, 2025
@fang-xing-esql
Copy link
Member Author

There are some unrelated failures in release tests, remove the test-release label for now.

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 8, 2026
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Left only some minor comments.

@nik9000
Copy link
Member

nik9000 commented Jan 13, 2026

  • HeapAttackSubqueryIT is added to catch potential OOM or CBE caused by large intermediate results from subqueries. Some tests with 8 subqueries hit OOM, instead of CBE, there are PR and new issues created to address those OOMs as follow ups. Currently most of the subquery heap attack tests run with 2 subqueries, root causes of the OOMs and TODOs and commented in each test.

Talked with @fang-xing-esql, all of the changes this PR powers are currently only available in SNAPSHOT builds. So it's quite safe to merge without fixing the OOMs. So long as we lock the removal of the SNAPSHOT behind fixing the OOMs, we're safe.

I'll look at the PR soon with that in mind.

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found an out of date comment, otherwise looks good to me.

@fang-xing-esql
Copy link
Member Author

Thanks for the review @astefan @nik9000 ! All the comments have been address.

@fang-xing-esql fang-xing-esql merged commit f207e27 into elastic:main Jan 14, 2026
36 checks passed
spinscale pushed a commit to spinscale/elasticsearch that referenced this pull request Jan 21, 2026
@fang-xing-esql fang-xing-esql deleted the remove-implicit-limit-from-unionall-branches branch January 30, 2026 14:38
alex-spies added a commit to alex-spies/elasticsearch that referenced this pull request Feb 2, 2026
9.3 does not have elastic#139058, so the implicit limits at the top of subquery
  branches are still in place. Adjust the expectations accordingly.
elasticsearchmachine pushed a commit that referenced this pull request Feb 2, 2026
…) (#141675)

* ESQL: Fix injected attributes's IDs in UnionAll branches (#141262)

This fixes the generation of name IDs for the attributes corresponding to the unmapped fields and are pushed to different branches in `UntionAll`.

So far, one set of IDs was generated and reused for all subplans. This is now updated to individual set per subplan. Along the change, the handling of `Fork` in `ResolveUnmapped` has been somewhat simplified.

Also, more unit tests have been completed (where the plans are simple enough) and the plan comments updated to replace the `EsqlProject` with the now merged `Project`.

A minor collateral proposed change: the CSV spec-based tests skipped due to missing capabilities are now logged.

(cherry picked from commit 8e3113c)

* Fix tests

9.3 does not have #139058, so the implicit limits at the top of subquery
  branches are still in place. Adjust the expectations accordingly.

* Checkstyle

---------

Co-authored-by: Bogdan Pintea <bogdan.pintea@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ES|QL] Remove the implicit LIMIT command added to each subquery (UnionAll branch)

4 participants