Skip to content

Commit 1b30a62

Browse files
committed
SQL: Fix issue with optimization on queries with ORDER BY/LIMIT (#40256)
Previously, when a trival plain `SELECT` or a trivial `SELECT` with aggregations has also an `ORDER BY` or a `LIMIT` or both, then the optimization to convert it to a `LocalRelation` was skipped resulting in exception thrown. E.g.:: ``` SELECT 'foo' FROM test LIMIT 10 ``` or ``` SELECT 'foo' FROM test GROUP BY 1 ORDER BY 1 ``` Fixes: #40211
1 parent 3a627e1 commit 1b30a62

File tree

3 files changed

+108
-7
lines changed

3 files changed

+108
-7
lines changed

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@
7575
import org.elasticsearch.xpack.sql.session.SingletonExecutable;
7676
import org.elasticsearch.xpack.sql.type.DataType;
7777
import org.elasticsearch.xpack.sql.util.CollectionUtils;
78+
import org.elasticsearch.xpack.sql.util.Holder;
7879

7980
import java.util.ArrayList;
8081
import java.util.Arrays;
@@ -1833,7 +1834,7 @@ static class SkipQueryOnLimitZero extends OptimizerRule<Limit> {
18331834
@Override
18341835
protected LogicalPlan rule(Limit limit) {
18351836
if (limit.limit() instanceof Literal) {
1836-
if (Integer.valueOf(0).equals((((Literal) limit.limit()).fold()))) {
1837+
if (Integer.valueOf(0).equals((limit.limit().fold()))) {
18371838
return new LocalRelation(limit.location(), new EmptyExecutable(limit.output()));
18381839
}
18391840
}
@@ -1844,21 +1845,30 @@ protected LogicalPlan rule(Limit limit) {
18441845
static class SkipQueryIfFoldingProjection extends OptimizerRule<LogicalPlan> {
18451846
@Override
18461847
protected LogicalPlan rule(LogicalPlan plan) {
1847-
if (plan instanceof Project) {
1848-
Project p = (Project) plan;
1848+
Holder<LocalRelation> optimizedPlan = new Holder<>();
1849+
plan.forEachDown(p -> {
18491850
List<Object> values = extractConstants(p.projections());
18501851
if (values.size() == p.projections().size() && !(p.child() instanceof EsRelation) &&
18511852
isNotQueryWithFromClauseAndFilterFoldedToFalse(p)) {
1852-
return new LocalRelation(p.location(), new SingletonExecutable(p.output(), values.toArray()));
1853+
optimizedPlan.set(new LocalRelation(p.location(), new SingletonExecutable(p.output(), values.toArray())));
18531854
}
1855+
}, Project.class);
1856+
1857+
if (optimizedPlan.get() != null) {
1858+
return optimizedPlan.get();
18541859
}
1855-
if (plan instanceof Aggregate) {
1856-
Aggregate a = (Aggregate) plan;
1860+
1861+
plan.forEachDown(a -> {
18571862
List<Object> values = extractConstants(a.aggregates());
18581863
if (values.size() == a.aggregates().size() && isNotQueryWithFromClauseAndFilterFoldedToFalse(a)) {
1859-
return new LocalRelation(a.location(), new SingletonExecutable(a.output(), values.toArray()));
1864+
optimizedPlan.set(new LocalRelation(a.location(), new SingletonExecutable(a.output(), values.toArray())));
18601865
}
1866+
}, Aggregate.class);
1867+
1868+
if (optimizedPlan.get() != null) {
1869+
return optimizedPlan.get();
18611870
}
1871+
18621872
return plan;
18631873
}
18641874

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
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+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
7+
package org.elasticsearch.xpack.sql.util;
8+
9+
/**
10+
* Simply utility class used for setting a state, typically
11+
* for closures (which require outside variables to be final).
12+
*/
13+
public class Holder<T> {
14+
15+
private T value = null;
16+
17+
public Holder() {
18+
}
19+
20+
public Holder(T value) {
21+
this.value = value;
22+
}
23+
24+
public void set(T value) {
25+
this.value = value;
26+
}
27+
28+
public T get() {
29+
return value;
30+
}
31+
}

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,36 @@ public void testFoldingToLocalExecWithProject() {
7070
assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
7171
}
7272

73+
public void testFoldingToLocalExecWithProjectAndLimit() {
74+
PhysicalPlan p = plan("SELECT keyword FROM test WHERE 1 = 2 LIMIT 10");
75+
assertEquals(LocalExec.class, p.getClass());
76+
LocalExec le = (LocalExec) p;
77+
assertEquals(EmptyExecutable.class, le.executable().getClass());
78+
EmptyExecutable ee = (EmptyExecutable) le.executable();
79+
assertEquals(1, ee.output().size());
80+
assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
81+
}
82+
83+
public void testFoldingToLocalExecWithProjectAndOrderBy() {
84+
PhysicalPlan p = plan("SELECT keyword FROM test WHERE 1 = 2 ORDER BY 1");
85+
assertEquals(LocalExec.class, p.getClass());
86+
LocalExec le = (LocalExec) p;
87+
assertEquals(EmptyExecutable.class, le.executable().getClass());
88+
EmptyExecutable ee = (EmptyExecutable) le.executable();
89+
assertEquals(1, ee.output().size());
90+
assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
91+
}
92+
93+
public void testFoldingToLocalExecWithProjectAndOrderByAndLimit() {
94+
PhysicalPlan p = plan("SELECT keyword FROM test WHERE 1 = 2 ORDER BY 1 LIMIT 10");
95+
assertEquals(LocalExec.class, p.getClass());
96+
LocalExec le = (LocalExec) p;
97+
assertEquals(EmptyExecutable.class, le.executable().getClass());
98+
EmptyExecutable ee = (EmptyExecutable) le.executable();
99+
assertEquals(1, ee.output().size());
100+
assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
101+
}
102+
73103
public void testLocalExecWithPrunedFilterWithFunction() {
74104
PhysicalPlan p = plan("SELECT E() FROM test WHERE PI() = 5");
75105
assertEquals(LocalExec.class, p.getClass());
@@ -90,6 +120,36 @@ public void testLocalExecWithPrunedFilterWithFunctionAndAggregation() {
90120
assertThat(ee.output().get(0).toString(), startsWith("E{c}#"));
91121
}
92122

123+
public void testFoldingToLocalExecWithAggregationAndLimit() {
124+
PhysicalPlan p = plan("SELECT 'foo' FROM test GROUP BY 1 LIMIT 10");
125+
assertEquals(LocalExec.class, p.getClass());
126+
LocalExec le = (LocalExec) p;
127+
assertEquals(SingletonExecutable.class, le.executable().getClass());
128+
SingletonExecutable ee = (SingletonExecutable) le.executable();
129+
assertEquals(1, ee.output().size());
130+
assertThat(ee.output().get(0).toString(), startsWith("foo{c}#"));
131+
}
132+
133+
public void testFoldingToLocalExecWithAggregationAndOrderBy() {
134+
PhysicalPlan p = plan("SELECT 'foo' FROM test GROUP BY 1 ORDER BY 1");
135+
assertEquals(LocalExec.class, p.getClass());
136+
LocalExec le = (LocalExec) p;
137+
assertEquals(SingletonExecutable.class, le.executable().getClass());
138+
SingletonExecutable ee = (SingletonExecutable) le.executable();
139+
assertEquals(1, ee.output().size());
140+
assertThat(ee.output().get(0).toString(), startsWith("foo{c}#"));
141+
}
142+
143+
public void testFoldingToLocalExecWithAggregationAndOrderByAndLimit() {
144+
PhysicalPlan p = plan("SELECT 'foo' FROM test GROUP BY 1 ORDER BY 1 LIMIT 10");
145+
assertEquals(LocalExec.class, p.getClass());
146+
LocalExec le = (LocalExec) p;
147+
assertEquals(SingletonExecutable.class, le.executable().getClass());
148+
SingletonExecutable ee = (SingletonExecutable) le.executable();
149+
assertEquals(1, ee.output().size());
150+
assertThat(ee.output().get(0).toString(), startsWith("foo{c}#"));
151+
}
152+
93153
public void testLocalExecWithoutFromClause() {
94154
PhysicalPlan p = plan("SELECT E(), 'foo', abs(10)");
95155
assertEquals(LocalExec.class, p.getClass());

0 commit comments

Comments
 (0)