Skip to content

Commit 3af3c11

Browse files
luigidellaquilaluigidellaquila
committed
Implement review suggestions
1 parent 3569a16 commit 3af3c11

File tree

6 files changed

+145
-151
lines changed

6 files changed

+145
-151
lines changed

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,15 +32,14 @@
3232
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneEmptyPlans;
3333
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneFilters;
3434
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneLiteralsInOrderBy;
35-
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneOrderByBeforeStats;
35+
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneRedundantOrderBy;
3636
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneRedundantSortClauses;
3737
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownAndCombineFilters;
3838
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownAndCombineLimits;
3939
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownAndCombineOrderBy;
4040
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownEnrich;
4141
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownEval;
4242
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownRegexExtract;
43-
import org.elasticsearch.xpack.esql.optimizer.rules.logical.RemoveRedundantSort;
4443
import org.elasticsearch.xpack.esql.optimizer.rules.logical.RemoveStatsOverride;
4544
import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceAggregateAggExpressionWithEval;
4645
import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceAggregateNestedExpressionWithEval;
@@ -190,12 +189,12 @@ protected static Batch<LogicalPlan> operators() {
190189
new PushDownRegexExtract(),
191190
new PushDownEnrich(),
192191
new PushDownAndCombineOrderBy(),
193-
new PruneOrderByBeforeStats(),
192+
new PruneRedundantOrderBy(),
194193
new PruneRedundantSortClauses()
195194
);
196195
}
197196

198197
protected static Batch<LogicalPlan> cleanup() {
199-
return new Batch<>("Clean Up", new ReplaceLimitAndSortAsTopN(), new ReplaceRowAsLocalRelation(), new RemoveRedundantSort());
198+
return new Batch<>("Clean Up", new ReplaceLimitAndSortAsTopN(), new ReplaceRowAsLocalRelation());
200199
}
201200
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneOrderByBeforeStats.java‎

Lines changed: 0 additions & 47 deletions
This file was deleted.

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.esql.optimizer.rules.logical;
9+
10+
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
11+
import org.elasticsearch.xpack.esql.plan.logical.Drop;
12+
import org.elasticsearch.xpack.esql.plan.logical.Enrich;
13+
import org.elasticsearch.xpack.esql.plan.logical.Eval;
14+
import org.elasticsearch.xpack.esql.plan.logical.Filter;
15+
import org.elasticsearch.xpack.esql.plan.logical.InlineStats;
16+
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
17+
import org.elasticsearch.xpack.esql.plan.logical.Lookup;
18+
import org.elasticsearch.xpack.esql.plan.logical.MvExpand;
19+
import org.elasticsearch.xpack.esql.plan.logical.OrderBy;
20+
import org.elasticsearch.xpack.esql.plan.logical.Project;
21+
import org.elasticsearch.xpack.esql.plan.logical.RegexExtract;
22+
import org.elasticsearch.xpack.esql.plan.logical.Rename;
23+
import org.elasticsearch.xpack.esql.plan.logical.TopN;
24+
import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan;
25+
import org.elasticsearch.xpack.esql.plan.logical.join.Join;
26+
27+
import java.util.ArrayList;
28+
import java.util.IdentityHashMap;
29+
import java.util.List;
30+
31+
/**
32+
* SORT cannot be executed without a LIMIT, as ES|QL doesn't support unbounded sort (yet).
33+
* <p>
34+
* The planner tries to push down LIMIT and transform all the unbounded sorts into a TopN.
35+
* In some cases it's not possible though, eg.
36+
* <p>
37+
* from test | sort x | lookup join lookup on x | sort y
38+
* <p>
39+
* from test | sort x | mv_expand x | sort y
40+
* <p>
41+
* "sort y" will become a TopN, but "sort x" will remain unbounded, so the query could not be executed.
42+
* <p>
43+
* In most cases though, following commands can make the previous SORTs redundant,
44+
* because it will re-sort previously sorted results (eg. if there is another SORT)
45+
* or because the order will be scrambled by another command (eg. a STATS)
46+
* <p>
47+
* This rule finds and prunes redundant SORTs, attempting to make the plan executable.
48+
*/
49+
public class PruneRedundantOrderBy extends OptimizerRules.OptimizerRule<LogicalPlan> {
50+
51+
@Override
52+
protected LogicalPlan rule(LogicalPlan plan) {
53+
if (plan instanceof OrderBy || plan instanceof TopN || plan instanceof Aggregate) {
54+
IdentityHashMap<OrderBy, Void> redundant = findRedundantSort(((UnaryPlan) plan).child());
55+
if (redundant.isEmpty()) {
56+
return plan;
57+
}
58+
return plan.transformUp(p -> {
59+
if (redundant.containsKey(p)) {
60+
return ((OrderBy) p).child();
61+
}
62+
return p;
63+
});
64+
} else {
65+
return plan;
66+
}
67+
}
68+
69+
private IdentityHashMap<OrderBy, Void> findRedundantSort(LogicalPlan plan) {
70+
List<LogicalPlan> toCheck = new ArrayList<>();
71+
toCheck.add(plan);
72+
73+
IdentityHashMap<OrderBy, Void> result = new IdentityHashMap<>();
74+
LogicalPlan p = null;
75+
while (true) {
76+
if (p == null) {
77+
if (toCheck.isEmpty()) {
78+
return result;
79+
} else {
80+
p = toCheck.remove(0);
81+
}
82+
} else if (p instanceof OrderBy ob) {
83+
result.put(ob, null);
84+
p = ob.child();
85+
} else if (p instanceof UnaryPlan unary) {
86+
if (unary instanceof Project
87+
|| unary instanceof Drop
88+
|| unary instanceof Rename
89+
|| unary instanceof MvExpand
90+
|| unary instanceof Enrich
91+
|| unary instanceof RegexExtract
92+
|| unary instanceof InlineStats
93+
|| unary instanceof Lookup
94+
// IMPORTANT
95+
// If we introduce window functions or order-sensitive aggs (eg. STREAMSTATS),
96+
// the previous sort could actually become relevant
97+
// so we have to be careful with plans that could use them, ie. the following
98+
|| unary instanceof Filter
99+
|| unary instanceof Eval
100+
|| unary instanceof Aggregate) {
101+
p = unary.child();
102+
} else {
103+
// stop here, other unary plans could be sensitive to SORT
104+
p = null;
105+
}
106+
} else if (p instanceof Join lj) {
107+
toCheck.add(lj.left());
108+
toCheck.add(lj.right());
109+
p = null;
110+
} else {
111+
// stop here, other unary plans could be sensitive to SORT
112+
p = null;
113+
}
114+
}
115+
}
116+
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/RemoveRedundantSort.java‎

Lines changed: 0 additions & 88 deletions
This file was deleted.

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,6 @@ public void postAnalysisVerification(Failures failures) {
113113

114114
@Override
115115
public void postOptimizationVerification(Failures failures) {
116-
failures.add(fail(this, "The query cannot be executed because it would require unbounded sort"));
116+
failures.add(fail(this, "Unbounded sort not supported yet, please add a limit"));
117117
}
118118
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java‎

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2219,8 +2219,7 @@ public void testPushDown_TheRightLimit_PastLookupJoin() {
22192219
* \_TopN[[Order[salary{f}#12,ASC,LAST]],5[INTEGER]]
22202220
* \_Eval[[100[INTEGER] AS b]]
22212221
* \_MvExpand[first_name{f}#11]
2222-
* \_TopN[[Order[first_name{f}#11,ASC,LAST]],10000[INTEGER]]
2223-
* \_EsRelation[employees][emp_no{f}#10, first_name{f}#11, salary{f}#12]
2222+
* \_EsRelation[employees][emp_no{f}#10, first_name{f}#11, salary{f}#12]
22242223
*/
22252224
public void testPushDownLimit_PastEvalAndMvExpand() {
22262225
LogicalPlan plan = optimizedPlan("""
@@ -2238,10 +2237,7 @@ public void testPushDownLimit_PastEvalAndMvExpand() {
22382237
assertThat(orderNames(topN), contains("salary"));
22392238
var eval = as(topN.child(), Eval.class);
22402239
var mvExp = as(eval.child(), MvExpand.class);
2241-
topN = as(mvExp.child(), TopN.class);
2242-
assertThat(topN.limit().fold(FoldContext.small()), equalTo(10000));
2243-
assertThat(orderNames(topN), contains("first_name"));
2244-
as(topN.child(), EsRelation.class);
2240+
as(mvExp.child(), EsRelation.class);
22452241
}
22462242

22472243
/**
@@ -7455,8 +7451,19 @@ public void testRedundantSortOnMvExpandJoinEnrichGrokDissect() {
74557451
as(eval.child(), EsRelation.class);
74567452
}
74577453

7458-
public void testUnboundedSort() throws Exception {
7459-
String query = """
7454+
/**
7455+
* TopN[[Order[emp_no{f}#15,ASC,LAST]],1000[INTEGER]]
7456+
* \_Filter[emp_no{f}#15 > 1[INTEGER]]
7457+
* \_MvExpand[foo{r}#10,foo{r}#29]
7458+
* \_Eval[[CONCAT(language_name{r}#28,[66 6f 6f][KEYWORD]) AS foo]]
7459+
* \_MvExpand[language_name{f}#27,language_name{r}#28]
7460+
* \_Join[LEFT,[language_code{r}#3],[language_code{r}#3],[language_code{f}#26]]
7461+
* |_Eval[[1[INTEGER] AS language_code]]
7462+
* | \_EsRelation[test][_meta_field{f}#21, emp_no{f}#15, first_name{f}#16, ..]
7463+
* \_EsRelation[languages_lookup][LOOKUP][language_code{f}#26, language_name{f}#27]
7464+
*/
7465+
public void testEvalLookupMultipleSorts() {
7466+
var plan = optimizedPlan("""
74607467
FROM test
74617468
| EVAL language_code = 1
74627469
| LOOKUP JOIN languages_lookup ON language_code
@@ -7466,9 +7473,16 @@ public void testUnboundedSort() throws Exception {
74667473
| MV_EXPAND foo
74677474
| WHERE emp_no > 1
74687475
| SORT emp_no
7469-
""";
7476+
""");
7477+
7478+
var topN = as(plan, TopN.class);
7479+
var filter = as(topN.child(), Filter.class);
7480+
var mvExpand = as(filter.child(), MvExpand.class);
7481+
var eval = as(mvExpand.child(), Eval.class);
7482+
mvExpand = as(eval.child(), MvExpand.class);
7483+
var join = as(mvExpand.child(), Join.class);
7484+
eval = as(join.left(), Eval.class);
7485+
as(eval.child(), EsRelation.class);
74707486

7471-
var e = expectThrows(VerificationException.class, () -> plan(query));
7472-
assertThat(e.getMessage(), is("Found 1 problem\nline 4:3: The query cannot be executed because it would require unbounded sort"));
74737487
}
74747488
}

0 commit comments

Comments
 (0)