ESQL: Compute output of LookupJoinExec dynamically #117763
Merged
Conversation
This file contains 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
LookupJoinExec should not assume its output but instead compute it from - Its input fields from the left - The fields added from the lookup index Currently, LookupJoinExec's output is determined when the logical plan is mapped to a physical one, and thereafter the output cannot be changed anymore. This makes it impossible to have late materialization of fields from the left hand side via field extractions, because we are forced to extract *all* fields before the LookupJoinExec, otherwise we do not achieve the prescribed output. Avoid that by tracking only which fields the LookupJoinExec will add from the lookup index instead of tracking the whole output (that was only correct for the logical plan).
alex-spies
commented
Nov 29, 2024
| @@ -62,34 +60,4 @@ public final PhysicalPlan apply(PhysicalPlan plan) { | |||
|
|
|||
| protected abstract PhysicalPlan rule(SubPlan plan); | |||
| } | |||
|
|
|||
| public abstract static class OptimizerExpressionRule<E extends Expression> extends Rule<PhysicalPlan, PhysicalPlan> { | |||
There was a problem hiding this comment.
Unrelated, but I noticed this became unused.
elasticsearchmachine
added
v9.0.0
needs:triage
alex-spies
commented
Nov 29, 2024
Comment on lines
-118
to
-122
| } else if (RIGHT.equals(joinType)) { | ||
| List<Attribute> leftOutputWithoutMatchFields = leftOutput.stream() | ||
| .filter(attr -> matchFieldNames.contains(attr.name()) == false) | ||
| .toList(); | ||
| output = mergeOutputAttributes(leftOutputWithoutMatchFields, rightOutput); |
There was a problem hiding this comment.
Unused, and it's a hassle to keep this in sync with the LEFT case.
alex-spies
commented
Nov 29, 2024
Comment on lines
+586
to
+587
| List<Layout.ChannelAndType> matchFields = new ArrayList<>(join.leftFields().size()); | ||
| for (Attribute m : join.leftFields()) { |
There was a problem hiding this comment.
For now, left fields and match fields are the same. In fact, the JoinConfig's matchFields is currently fully redundant.
alex-spies
added
>non-issue
auto-backport
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
craigtaverner
approved these changes
Nov 29, 2024
| this.leftFields = in.readNamedWriteableCollectionAsList(Attribute.class); | ||
| this.rightFields = in.readNamedWriteableCollectionAsList(Attribute.class); | ||
| this.output = in.readNamedWriteableCollectionAsList(Attribute.class); | ||
| this.addedFields = in.readNamedWriteableCollectionAsList(Attribute.class); |
There was a problem hiding this comment.
I presume we're not bothering about transport version because this is a snapshot only feature right now?
There was a problem hiding this comment.
We could avoid breaking bwc with a new transport version, but the previous version of LOOKUP JOIN isn't exactly fully working either, so I don't think we need to bother.
alex-spies
added
the
auto-merge-without-approval
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
alex-spies
added a commit
to alex-spies/elasticsearch
that referenced
this pull request
Nov 29, 2024
LookupJoinExec should not assume its output but instead compute it from - Its input fields from the left - The fields added from the lookup index Currently, LookupJoinExec's output is determined when the logical plan is mapped to a physical one, and thereafter the output cannot be changed anymore. This makes it impossible to have late materialization of fields from the left hand side via field extractions, because we are forced to extract *all* fields before the LookupJoinExec, otherwise we do not achieve the prescribed output. Avoid that by tracking only which fields the LookupJoinExec will add from the lookup index instead of tracking the whole output (that was only correct for the logical plan). **Note:** While this PR is a refactoring for the current functionality, it should unblock @craigtaverner 's ongoing work related to field extractions and getting multiple LOOKUP JOIN queries to work correctly without adding hacks. (cherry picked from commit 64107e0) # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java
elasticsearchmachine
pushed a commit
that referenced
this pull request
Dec 2, 2024
LookupJoinExec should not assume its output but instead compute it from - Its input fields from the left - The fields added from the lookup index Currently, LookupJoinExec's output is determined when the logical plan is mapped to a physical one, and thereafter the output cannot be changed anymore. This makes it impossible to have late materialization of fields from the left hand side via field extractions, because we are forced to extract *all* fields before the LookupJoinExec, otherwise we do not achieve the prescribed output. Avoid that by tracking only which fields the LookupJoinExec will add from the lookup index instead of tracking the whole output (that was only correct for the logical plan). **Note:** While this PR is a refactoring for the current functionality, it should unblock @craigtaverner 's ongoing work related to field extractions and getting multiple LOOKUP JOIN queries to work correctly without adding hacks. (cherry picked from commit 64107e0) # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java
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.
LookupJoinExec should not assume its output but instead compute it from
Currently, LookupJoinExec's output is determined when the logical plan is mapped to a physical one, and thereafter the output cannot be changed anymore.
This makes it impossible to have late materialization of fields from the left hand side via field extractions, because we are forced to extract all fields before the LookupJoinExec, otherwise we do not achieve the prescribed output.
Avoid that by tracking only which fields the LookupJoinExec will add from the lookup index instead of tracking the whole output (that was only correct for the logical plan).
Note: While this PR is a refactoring for the current functionality, it should unblock @craigtaverner 's ongoing work related to field extractions and getting multiple LOOKUP JOIN queries to work correctly without adding hacks.