-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
ESQL: mv_expand pushes down limit and project and keep the limit after it untouched #100782
Changes from 1 commit
29a85c3
7f8b9a0
e303b50
aadba96
6259816
c27f9c7
4a67590
90f3edf
8fafb07
ecd71ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.In; | ||
import org.elasticsearch.xpack.esql.plan.logical.Enrich; | ||
import org.elasticsearch.xpack.esql.plan.logical.Eval; | ||
import org.elasticsearch.xpack.esql.plan.logical.MvExpand; | ||
import org.elasticsearch.xpack.esql.plan.logical.RegexExtract; | ||
import org.elasticsearch.xpack.esql.plan.logical.TopN; | ||
import org.elasticsearch.xpack.esql.plan.logical.local.EsqlProject; | ||
|
@@ -126,6 +127,7 @@ protected static List<Batch<LogicalPlan>> rules() { | |
new PushDownEval(), | ||
new PushDownRegexExtract(), | ||
new PushDownEnrich(), | ||
new PushDownMvExpand(), | ||
new PushDownAndCombineOrderBy(), | ||
new PruneOrderByBeforeStats(), | ||
new PruneRedundantSortClauses() | ||
|
@@ -383,7 +385,8 @@ protected LogicalPlan rule(Limit limit) { | |
// check if there's a 'visible' descendant limit lower than the current one | ||
// and if so, align the current limit since it adds no value | ||
// this applies for cases such as | limit 1 | sort field | limit 10 | ||
else { | ||
// but NOT for mv_expand (ie | limit 1 | mv_expand x | limit 20) where we want that last "limit" to apply on expand results | ||
else if (unary instanceof MvExpand == false) { | ||
Limit descendantLimit = descendantLimit(unary); | ||
if (descendantLimit != null) { | ||
var l1 = (int) limit.limit().fold(); | ||
|
@@ -653,8 +656,22 @@ protected LogicalPlan rule(Enrich re) { | |
} | ||
} | ||
|
||
protected static class PushDownAndCombineOrderBy extends OptimizerRules.OptimizerRule<OrderBy> { | ||
protected static class PushDownMvExpand extends OptimizerRules.OptimizerRule<MvExpand> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the advantage of pushing down MvExpand? I'd argue we want the opposite as this increases the amount of data earlier in the pipeline. |
||
@Override | ||
protected LogicalPlan rule(MvExpand mve) { | ||
LogicalPlan child = mve.child(); | ||
|
||
if (child instanceof OrderBy orderBy) { | ||
return orderBy.replaceChild(mve.replaceChild(orderBy.child())); | ||
} else if (child instanceof Project) { | ||
return pushDownPastProject(mve); | ||
} | ||
|
||
return mve; | ||
} | ||
} | ||
|
||
protected static class PushDownAndCombineOrderBy extends OptimizerRules.OptimizerRule<OrderBy> { | ||
@Override | ||
protected LogicalPlan rule(OrderBy orderBy) { | ||
LogicalPlan child = orderBy.child(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ | |
import org.elasticsearch.xpack.esql.plan.logical.Enrich; | ||
import org.elasticsearch.xpack.esql.plan.logical.Eval; | ||
import org.elasticsearch.xpack.esql.plan.logical.Grok; | ||
import org.elasticsearch.xpack.esql.plan.logical.MvExpand; | ||
import org.elasticsearch.xpack.esql.plan.logical.TopN; | ||
import org.elasticsearch.xpack.esql.plan.logical.local.EsqlProject; | ||
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation; | ||
|
@@ -888,6 +889,50 @@ public void testCombineOrderByThroughFilter() { | |
as(filter.child(), EsRelation.class); | ||
} | ||
|
||
public void testCombineOrderByThroughMvExpand() { | ||
LogicalPlan plan = optimizedPlan(""" | ||
from test | ||
| sort emp_no | ||
| mv_expand first_name | ||
| sort first_name"""); | ||
|
||
var topN = as(plan, TopN.class); | ||
assertThat(orderNames(topN), contains("first_name", "emp_no")); | ||
var mvExpand = as(topN.child(), MvExpand.class); | ||
as(mvExpand.child(), EsRelation.class); | ||
} | ||
|
||
public void testPushDownMvExpandPastProject() { | ||
LogicalPlan plan = optimizedPlan(""" | ||
from test | ||
| rename first_name as x | ||
| keep x | ||
| mv_expand x | ||
"""); | ||
|
||
var keep = as(plan, Project.class); | ||
var limit = as(keep.child(), Limit.class); | ||
var mvExpand = as(limit.child(), MvExpand.class); | ||
assertThat(as(mvExpand.target(), FieldAttribute.class).name(), is("first_name")); | ||
} | ||
|
||
public void testDontPushDownLimitPastMvExpand() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add as javadoc the expanded plans. |
||
LogicalPlan plan = optimizedPlan(""" | ||
from test | ||
| limit 1 | ||
| keep first_name, last_name | ||
| mv_expand first_name | ||
| limit 10"""); | ||
|
||
var project = as(plan, Project.class); | ||
var limit = as(project.child(), Limit.class); | ||
assertThat(limit.limit().fold(), equalTo(10)); | ||
var mvExpand = as(limit.child(), MvExpand.class); | ||
limit = as(mvExpand.child(), Limit.class); | ||
assertThat(limit.limit().fold(), equalTo(1)); | ||
as(limit.child(), EsRelation.class); | ||
} | ||
|
||
private static List<String> orderNames(TopN topN) { | ||
return topN.order().stream().map(o -> as(o.child(), NamedExpression.class).name()).toList(); | ||
} | ||
|
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.
Don't combine the limits for mv_expand.
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.
this works only if the limit is right above
mv_expand
- if it's hidden by another node (keep, where, etc..) the rule won't see it.Move the check into descendantLimit so that if mv_expand is encountered, just like with an agg, no limit is returned.
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.
Good point. Haven't thought of this scenario.