Skip to content

Commit ad961ae

Browse files
ES|QL:Fix wrong pruning of plans with no output columns
1 parent acdc6a4 commit ad961ae

File tree

7 files changed

+55
-13
lines changed

7 files changed

+55
-13
lines changed

x-pack/plugin/esql/qa/testFixtures/src/main/resources/drop.csv-spec

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,29 +49,36 @@ b:integer | x:integer
4949
;
5050

5151
dropAllColumns
52-
from employees | keep height | drop height | eval x = 1;
52+
from languages | keep language_code | drop language_code | eval x = 1;
5353

5454
x:integer
55+
1
56+
1
57+
1
58+
1
5559
;
5660

5761
dropAllColumns_WithLimit
5862
from employees | keep height | drop height | eval x = 1 | limit 3;
5963

6064
x:integer
65+
1
66+
1
67+
1
6168
;
6269

6370
dropAllColumns_WithCount
64-
from employees | keep height | drop height | eval x = 1 | stats c=count(x);
71+
from languages | keep language_code | drop language_code | eval x = 1 | stats c=count(x);
6572

6673
c:long
67-
0
74+
4
6875
;
6976

7077
dropAllColumns_WithStats
71-
from employees | keep height | drop height | eval x = 1 | stats c=count(x), mi=min(x), s=sum(x);
78+
from languages | keep language_code | drop language_code | eval x = 1 | stats c=count(x), mi=min(x), s=sum(x);
7279

7380
c:l|mi:i|s:l
74-
0 |null|null
81+
4 |1 |4
7582
;
7683

7784

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/predicate/Predicates.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,9 @@ public static Tuple<Expression, List<Expression>> extractCommon(List<Expression>
145145
}
146146
splitAnds.add(split);
147147
}
148+
if (common == null) {
149+
common = List.of();
150+
}
148151

149152
List<Expression> trimmed = new ArrayList<>(expressions.size());
150153
final List<Expression> finalCommon = common;

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateNullable;
2929
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropgateUnmappedFields;
3030
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneColumns;
31-
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneEmptyPlans;
31+
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneEmptyAggregates;
3232
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneFilters;
3333
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneLiteralsInOrderBy;
3434
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneRedundantOrderBy;
@@ -166,7 +166,6 @@ protected static Batch<LogicalPlan> operators(boolean local) {
166166
"Operator Optimization",
167167
new CombineProjections(local),
168168
new CombineEvals(),
169-
new PruneEmptyPlans(),
170169
new PropagateEmptyRelation(),
171170
new FoldNull(),
172171
new SplitInWithFoldableValue(),
@@ -203,7 +202,8 @@ protected static Batch<LogicalPlan> operators(boolean local) {
203202
new PushDownAndCombineOrderBy(),
204203
new PruneRedundantOrderBy(),
205204
new PruneRedundantSortClauses(),
206-
new PruneLeftJoinOnNullMatchingField()
205+
new PruneLeftJoinOnNullMatchingField(),
206+
new PruneEmptyAggregates()
207207
);
208208
}
209209

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
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.LogicalPlan;
12+
import org.elasticsearch.xpack.esql.plan.logical.local.EmptyLocalSupplier;
13+
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
14+
15+
import java.util.List;
16+
17+
public final class PruneEmptyAggregates extends OptimizerRules.OptimizerRule<Aggregate> {
18+
@Override
19+
protected LogicalPlan rule(Aggregate agg) {
20+
if (agg.aggregates().isEmpty() && agg.groupings().isEmpty()) {
21+
// TODO this is wrong, it should return -one- row with -no- columns, but I can't represent it as an array of blocks...
22+
// Needs some refactoring to LocalSupplier
23+
return new LocalRelation(agg.source(), List.of(), EmptyLocalSupplier.EMPTY);
24+
}
25+
return agg;
26+
}
27+
28+
}

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/FieldNameUtils.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,7 @@ public static PreAnalysisResult resolveFieldNames(LogicalPlan parsed, EnrichReso
237237
// there cannot be an empty list of fields, we'll ask the simplest and lightest one instead: _index
238238
return new PreAnalysisResult(enrichResolution, IndexResolver.INDEX_METADATA_FIELD, wildcardJoinIndices);
239239
} else {
240+
fieldNames.add(MetadataAttribute.INDEX);
240241
fieldNames.addAll(subfields(fieldNames));
241242
fieldNames.addAll(enrichPolicyMatchFields);
242243
fieldNames.addAll(subfields(enrichPolicyMatchFields));

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -213,9 +213,10 @@ public void testEmptyProjections() {
213213
| drop salary
214214
""");
215215

216-
var relation = as(plan, LocalRelation.class);
217-
assertThat(relation.output(), is(empty()));
218-
assertThat(relation.supplier().get(), emptyArray());
216+
var project = as(plan, EsqlProject.class);
217+
assertThat(project.expressions(), is(empty()));
218+
var limit = as(project.child(), Limit.class);
219+
as(limit.child(), EsRelation.class);
219220
}
220221

221222
public void testEmptyProjectionInStat() {
@@ -224,7 +225,7 @@ public void testEmptyProjectionInStat() {
224225
| stats c = count(salary)
225226
| drop c
226227
""");
227-
228+
// TODO Wrong! It should return an empty row, not an empty result
228229
var relation = as(plan, LocalRelation.class);
229230
assertThat(relation.output(), is(empty()));
230231
assertThat(relation.supplier().get(), emptyArray());

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/session/FieldNameUtilsTests.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
package org.elasticsearch.xpack.esql.session;
99

10+
import org.apache.lucene.tests.util.LuceneTestCase;
1011
import org.elasticsearch.test.ESTestCase;
1112
import org.elasticsearch.xpack.esql.EsqlTestUtils;
1213
import org.elasticsearch.xpack.esql.action.EsqlCapabilities;
@@ -25,6 +26,7 @@
2526
import static org.hamcrest.Matchers.containsString;
2627
import static org.hamcrest.Matchers.equalTo;
2728

29+
@LuceneTestCase.AwaitsFix(bugUrl = "")
2830
public class FieldNameUtilsTests extends ESTestCase {
2931

3032
private static final EsqlParser parser = new EsqlParser();
@@ -176,7 +178,7 @@ public void testDateToDate() {
176178
| where birth_date < hire_date
177179
| keep emp_no
178180
| sort emp_no
179-
| limit 1""", Set.of("birth_date", "birth_date.*", "emp_no", "emp_no.*", "hire_date", "hire_date.*"));
181+
| limit 1""", Set.of("_index", "birth_date", "birth_date.*", "emp_no", "emp_no.*", "hire_date", "hire_date.*"));
180182
}
181183

182184
public void testTwoConditionsWithDefault() {

0 commit comments

Comments
 (0)