Skip to content

ES|QL: Push limit to FORK branches#139443

Merged
ioanatia merged 5 commits intoelastic:mainfrom
ioanatia:fork_optim
Dec 15, 2025
Merged

ES|QL: Push limit to FORK branches#139443
ioanatia merged 5 commits intoelastic:mainfrom
ioanatia:fork_optim

Conversation

@ioanatia
Copy link
Copy Markdown
Member

@ioanatia ioanatia commented Dec 12, 2025 •
edited
Loading

part of #136820

Queries using FORK can be further optimized when a LIMIT is used after FORK:

from employees
| fork (where emp_no > 100 | SORT first_name | limit 5)
          (where emp_no < 10  | SORT first_name)
| eval x = 1
| mv_expand emp_no
| limit 10

we have existing optimizations that move the limit past EVAL and add limits before MV_EXPAND:

from employees
| fork (where emp_no > 100 | SORT first_name | limit 5)
          (where emp_no < 10  | SORT first_name)
| limit 10
| eval x = 1
| mv_expand emp_no
| limit 10

in this PR when we detect that a LIMIT follows a FORK, we attempt to duplicate the FORK limit in the branches, and so the final plan becomes something closer to:

from employees
| fork (where emp_no > 100 | SORT first_name | limit 5) // the limit here is smaller than the one outside of fork
          (where emp_no < 10  | SORT first_name | limit 10)
| limit 10 // the limit outside of FORK remains
| eval x = 1
| mv_expand emp_no
| limit 10

This optimization will be useful when we will remove the implicit limit we add right now in the FORK branches.

@ioanatia ioanatia added >non-issue Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch :Search Relevance/ES|QL Search functionality in ES|QL v9.3.0 labels Dec 12, 2025
@ioanatia ioanatia mentioned this pull request Dec 12, 2025
Open
3 tasks
ioanatia
ioanatia commented Dec 12, 2025
View reviewed changes
...main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java
}

private static LogicalPlan maybePushDownLimitToForkBranch(Limit limit, LogicalPlan forkBranch, LogicalOptimizerContext ctx) {
if (forkBranch instanceof UnaryPlan == false) {
Copy link
Copy Markdown
Member Author

@ioanatia ioanatia Dec 12, 2025

Choose a reason for hiding this comment

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

I am using the descendantLimit helper which needs a UnaryPlan and this is why we have this check here.
When we remove the implicit limit for FORK, we might need to modify this further and not rely on descendantLimit

@ioanatia ioanatia marked this pull request as ready for review December 12, 2025 15:24
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Dec 12, 2025

Pinging @elastic/es-search-relevance (Team:Search Relevance)

pmpailis
pmpailis approved these changes Dec 15, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@pmpailis pmpailis left a comment

Choose a reason for hiding this comment

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

LGTM; only a minor comment on an equality check

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java Outdated

for (LogicalPlan forkChild : fork.children()) {
LogicalPlan newForkChild = maybePushDownLimitToForkBranch(limit, forkChild, ctx);
changed = changed || newForkChild != limit;
Copy link
Copy Markdown
Contributor

@pmpailis pmpailis Dec 15, 2025

Choose a reason for hiding this comment

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

Shouldn't this be newForkChild != forkChild ?

Copy link
Copy Markdown
Member Author

@ioanatia ioanatia Dec 15, 2025

Choose a reason for hiding this comment

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

yes - good catch! 😌

carlosdelest
carlosdelest approved these changes Dec 15, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@carlosdelest carlosdelest left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I think we should add a CSV / integration test as well to have an end-to-end test.

@ioanatia ioanatia merged commit f11cb4a into elastic:main Dec 15, 2025
36 checks passed
@ioanatia ioanatia deleted the fork_optim branch December 15, 2025 15:40
parkertimmins pushed a commit to parkertimmins/elasticsearch that referenced this pull request Dec 17, 2025
@ioanatia @parkertimmins
ES|QL: Push limit to FORK branches (elastic#139443)
0898c12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlosdelest carlosdelest carlosdelest approved these changes

@pmpailis pmpailis pmpailis approved these changes

@afoucret afoucret Awaiting requested review from afoucret

Assignees

No one assigned

Labels

>non-issue :Search Relevance/ES|QL Search functionality in ES|QL Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v9.3.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ioanatia @elasticsearchmachine @pmpailis @carlosdelest