-
Notifications
You must be signed in to change notification settings - Fork 25.7k
ESQL: Push down MvExpand past Project
#136398
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
Open
kanoshiou
wants to merge
27
commits into
elastic:main
Choose a base branch
from
kanoshiou:push-down-mv_expand-past-project
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
ca954c4
Push down MvExpand past Project
kanoshiou 55c37c2
Update docs/changelog/136398.yaml
kanoshiou 91f86d6
Merge remote-tracking branch 'origin/main' into push-down-mv_expand-p…
kanoshiou a8b3897
Insert an eval if target in mv_expand is aliased in child project
kanoshiou 83aa387
Add tests for #136596
kanoshiou 2095c44
Update docs/changelog/136398.yaml
kanoshiou 8d64398
Merge branch 'refs/heads/main' into push-down-mv_expand-past-project
kanoshiou a46cc7d
Merge branch 'refs/heads/main' into push-down-mv_expand-past-project
kanoshiou 471124c
Merge branch 'refs/heads/main' into push-down-mv_expand-past-project
kanoshiou d08a6ea
refactor(esql): Improve MvExpand push-down logic in logical optimizer
kanoshiou 075e826
refactor(esql): Improve MvExpand push-down logic in logical optimizer
kanoshiou 6018ce0
Merge branch 'refs/heads/main' into push-down-mv_expand-past-project
kanoshiou 3afecaa
refactor(esql): Improve MvExpand push-down logic in logical optimizer
kanoshiou 2b3d55a
refactor(esql): Improve MvExpand push-down logic in logical optimizer
kanoshiou 8e69eb0
Merge branch 'main' into push-down-mv_expand-past-project
kanoshiou faea78b
refactor(esql): Improve MvExpand push-down logic in PushDownMvExpandP…
kanoshiou 7f8faf6
Merge branch 'refs/heads/main' into push-down-mv_expand-past-project
kanoshiou 92dacb5
fix: refine `PushDownMvExpandPastProject` logic by correcting the ali…
kanoshiou eb3f373
Merge branch 'refs/heads/main' into push-down-mv_expand-past-project
kanoshiou adab9f8
fix(esql): simplify MvExpand pushdown logic by extracting to utility …
kanoshiou dd86f32
Update test
kanoshiou b042096
fix(esql): refactor MvExpand pushdown logic to directly manipulate pr…
kanoshiou d8c221a
Avoid inconsistent plans
kanoshiou b5ede88
Create a temporary attribute for the UnionAll case
kanoshiou cd844d0
Add more tests for UnionAll cases
kanoshiou 2c45495
Merge branch 'refs/heads/main' into push-down-mv_expand-past-project
kanoshiou 9410846
Merge branch 'refs/heads/main' into push-down-mv_expand-past-project
kanoshiou File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| pr: 136398 | ||
| summary: "ESQL: Push down `MvExpand` past `Project`" | ||
| area: ES|QL | ||
| type: enhancement | ||
| issues: | ||
| - 136292 |
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
32 changes: 32 additions & 0 deletions
32
...ava/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownMvExpandPastProject.java
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| package org.elasticsearch.xpack.esql.optimizer.rules.logical; | ||
|
|
||
| import org.elasticsearch.xpack.esql.core.expression.Alias; | ||
| import org.elasticsearch.xpack.esql.core.expression.AttributeMap; | ||
| import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; | ||
| import org.elasticsearch.xpack.esql.plan.logical.MvExpand; | ||
| import org.elasticsearch.xpack.esql.plan.logical.Project; | ||
|
|
||
| public final class PushDownMvExpandPastProject extends OptimizerRules.OptimizerRule<MvExpand> { | ||
| @Override | ||
| protected LogicalPlan rule(MvExpand mvExpand) { | ||
| if (mvExpand.child() instanceof Project pj | ||
| && pj.projections() | ||
| .stream() | ||
| .anyMatch( | ||
| p -> p instanceof Alias alias | ||
| && (alias.toAttribute().equals(mvExpand.target().toAttribute()) | ||
| || alias.references().contains(mvExpand.target().toAttribute())) | ||
| ) == false) { | ||
| Project project = PushDownUtils.pushDownPastProject(mvExpand); | ||
| return PushDownUtils.resolveRenamesFromMap(project, AttributeMap.of(mvExpand.target().toAttribute(), mvExpand.expanded())); | ||
kanoshiou marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| return mvExpand; | ||
| } | ||
| } | ||
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be simpler to just put a brand new projection at the top based on
mvExpand.output(), but plugging back in the aliases that the original projection created. That's how we do it in ReplaceAliasingEvalWithProject (although that code is a little complex to use as reference :/ )There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to take my own advice back, sorry! Given that we already update the list of
projections, I think going back tomvExpand.output()makes the code more confusing, and it's likely simpler to just use the updatedprojectionslist directly to create the newProjectionthat goes on top of themvExpand.