Skip to content

ES|QL: Support LOOKUP JOIN with FORK #128839

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 4, 2025
Merged

Conversation

ioanatia
Copy link
Contributor

@ioanatia ioanatia commented Jun 3, 2025

related: #121950

This removes a TODO for fixing ProjectAwayColumns for FORK, which also fixes support for using LOOKUP JOIN and FORK.

It was reported that when we using LOOKUP JOIN in combination with FORK, this results in an error:

Example:

FROM employees
| EVAL language_code = languages
| LOOKUP JOIN languages_lookup ON language_code
| FORK (EVAL x = 1) ( EVAL y = 1)
| keep l*

results in org.elasticsearch.xpack.esql.plan.physical.ProjectExec cannot be cast to class org.elasticsearch.xpack.esql.plan.physical.EsQueryExec

This is raised here in LocalExecutionPlanner#L702:

private PhysicalOperation planLookupJoin(LookupJoinExec join, LocalExecutionPlannerContext context) {
PhysicalOperation source = plan(join.left(), context);
Layout.Builder layoutBuilder = source.layout.builder();
for (Attribute f : join.addedFields()) {
layoutBuilder.append(f);
}
Layout layout = layoutBuilder.build();
EsQueryExec localSourceExec = (EsQueryExec) join.lookup();

Normally, the right child of LookUpJoinExec is a FragmentExec that wraps a EsRelation.
Then this FragmentExec gets mapped to a EsQueryExec.

However, when using FORK, when we split the plan into sub plans, we add a Project node inside each FragmentExec which causes issues when using LOOKUP JOIN in combination with FORK.

Notice the TODO:

public static Tuple<List<PhysicalPlan>, PhysicalPlan> breakPlanIntoSubPlansAndMainPlan(PhysicalPlan plan) {
var subplans = new Holder<List<PhysicalPlan>>();
PhysicalPlan mainPlan = plan.transformUp(MergeExec.class, me -> {
subplans.set(me.children().stream().map(child -> {
// TODO: we are adding a Project plan to force InsertFieldExtraction - we should remove this transformation
child = child.transformUp(FragmentExec.class, f -> {
var logicalFragment = f.fragment();
logicalFragment = new Project(logicalFragment.source(), logicalFragment, logicalFragment.output());
return new FragmentExec(logicalFragment);
});

We have done this because we needed to fix ProjectAwayColumns, where we simply skip MergeExec (the physical plan for FORK):

// This will require updating should we choose to have non-unary execution plans in the future.
return plan.transformDown(currentPlanNode -> {
if (currentPlanNode instanceof MergeExec) {
keepTraversing.set(FALSE);
}

The fix was therefore to remove the hack from PlannerUtils.breakPlanIntoSubPlansAndMainPlan and to make sure that ProjectAwayColumns works correctly when FORK is used.

@ioanatia ioanatia added >non-issue :Analytics/ES|QL AKA ESQL v9.1.0 Team:Search - Relevance The Search organization Search Relevance team labels Jun 3, 2025
@ioanatia ioanatia changed the title Support LOOKUP JOIN with FORK ES|QL: Support LOOKUP JOIN with FORK Jun 3, 2025
@ioanatia ioanatia mentioned this pull request Jun 3, 2025
16 tasks
@ioanatia ioanatia marked this pull request as ready for review June 4, 2025 07:48
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) and removed Team:Search - Relevance The Search organization Search Relevance team labels Jun 4, 2025
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tteofili tteofili left a comment

Choose a reason for hiding this comment

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

LGTM

@ioanatia ioanatia merged commit ca904ce into elastic:main Jun 4, 2025
18 checks passed
@ioanatia ioanatia deleted the fork_lookup_join branch June 4, 2025 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants