Skip to content

Commit a399e29

Browse files
ES|QL: Fix wrong pruning of plans with no output columns (#133405)
1 parent abf225c commit a399e29

File tree

40 files changed

+1121
-441
lines changed

40 files changed

+1121
-441
lines changed

docs/changelog/133405.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 133405
2+
summary: Fix wrong pruning of plans with no output columns
3+
area: ES|QL
4+
type: bug
5+
issues: []
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
9186000
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
initial_9.2.0,9185000
1+
esql_plan_with_no_columns,9186000

x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/BasicPageTests.java

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,15 @@ public void testEqualityAndHashCodeSmallInput() {
5151
);
5252
in.releaseBlocks();
5353

54+
in = new Page(10);
55+
EqualsHashCodeTestUtils.checkEqualsAndHashCode(
56+
in,
57+
page -> new Page(10),
58+
page -> new Page(8, blockFactory.newConstantIntBlockWith(1, 8)),
59+
Page::releaseBlocks
60+
);
61+
in.releaseBlocks();
62+
5463
in = new Page(blockFactory.newIntArrayVector(new int[] {}, 0).asBlock());
5564
EqualsHashCodeTestUtils.checkEqualsAndHashCode(
5665
in,
@@ -133,8 +142,8 @@ public void testEqualityAndHashCode() throws IOException {
133142
return new Page(blocks);
134143
};
135144

136-
int positions = randomIntBetween(1, 512);
137-
int blockCount = randomIntBetween(1, 256);
145+
int positions = randomIntBetween(0, 512);
146+
int blockCount = randomIntBetween(0, 256);
138147
Block[] blocks = new Block[blockCount];
139148
for (int blockIndex = 0; blockIndex < blockCount; blockIndex++) {
140149
blocks[blockIndex] = switch (randomInt(9)) {
@@ -198,6 +207,16 @@ public void testAppend() {
198207
page2.releaseBlocks();
199208
}
200209

210+
public void testAppendToEmpty() {
211+
Page page1 = new Page(10);
212+
Page page2 = page1.appendBlock(blockFactory.newLongArrayVector(LongStream.range(0, 10).toArray(), 10).asBlock());
213+
assertThat(0, is(page1.getBlockCount()));
214+
assertThat(1, is(page2.getBlockCount()));
215+
LongBlock block1 = page2.getBlock(0);
216+
IntStream.range(0, 10).forEach(i -> assertThat((long) i, is(block1.getLong(i))));
217+
page2.releaseBlocks();
218+
}
219+
201220
public void testPageSerializationSimple() throws IOException {
202221
IntVector toFilter = blockFactory.newIntArrayVector(IntStream.range(0, 20).toArray(), 20);
203222
Page origPage = new Page(
@@ -248,6 +267,22 @@ public void testPageSerializationSimple() throws IOException {
248267
}
249268
}
250269

270+
public void testPageSerializationEmpty() throws IOException {
271+
Page origPage = new Page(10);
272+
try {
273+
Page deserPage = serializeDeserializePage(origPage);
274+
try {
275+
EqualsHashCodeTestUtils.checkEqualsAndHashCode(origPage, unused -> deserPage);
276+
assertEquals(origPage.getBlockCount(), deserPage.getBlockCount());
277+
assertEquals(origPage.getPositionCount(), deserPage.getPositionCount());
278+
} finally {
279+
deserPage.releaseBlocks();
280+
}
281+
} finally {
282+
origPage.releaseBlocks();
283+
}
284+
}
285+
251286
public void testSerializationListPages() throws IOException {
252287
final int positions = randomIntBetween(1, 64);
253288
List<Page> origPages = List.of(
@@ -265,7 +300,8 @@ public void testSerializationListPages() throws IOException {
265300
positions
266301
)
267302
),
268-
new Page(blockFactory.newConstantBytesRefBlockWith(new BytesRef("Hello World"), positions))
303+
new Page(blockFactory.newConstantBytesRefBlockWith(new BytesRef("Hello World"), positions)),
304+
new Page(10)
269305
);
270306
try {
271307
EqualsHashCodeTestUtils.checkEqualsAndHashCode(origPages, page -> {

x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.elasticsearch.compute.data.DoubleBlock;
3737
import org.elasticsearch.compute.data.IntBlock;
3838
import org.elasticsearch.compute.data.LongBlock;
39+
import org.elasticsearch.compute.data.Page;
3940
import org.elasticsearch.compute.lucene.DataPartitioning;
4041
import org.elasticsearch.core.PathUtils;
4142
import org.elasticsearch.core.SuppressForbidden;
@@ -498,7 +499,11 @@ public static LogicalPlan emptySource() {
498499
}
499500

500501
public static LogicalPlan localSource(BlockFactory blockFactory, List<Attribute> fields, List<Object> row) {
501-
return new LocalRelation(Source.EMPTY, fields, LocalSupplier.of(BlockUtils.fromListRow(blockFactory, row)));
502+
return new LocalRelation(
503+
Source.EMPTY,
504+
fields,
505+
LocalSupplier.of(row.isEmpty() ? new Page(0) : new Page(BlockUtils.fromListRow(blockFactory, row)))
506+
);
502507
}
503508

504509
public static <T> T as(Object node, Class<T> type) {

x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/generator/EsqlQueryGenerator.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import org.elasticsearch.xpack.esql.generator.command.CommandGenerator;
1212
import org.elasticsearch.xpack.esql.generator.command.pipe.ChangePointGenerator;
1313
import org.elasticsearch.xpack.esql.generator.command.pipe.DissectGenerator;
14+
import org.elasticsearch.xpack.esql.generator.command.pipe.DropAllGenerator;
1415
import org.elasticsearch.xpack.esql.generator.command.pipe.DropGenerator;
1516
import org.elasticsearch.xpack.esql.generator.command.pipe.EnrichGenerator;
1617
import org.elasticsearch.xpack.esql.generator.command.pipe.EvalGenerator;
@@ -63,6 +64,7 @@ public class EsqlQueryGenerator {
6364
ChangePointGenerator.INSTANCE,
6465
DissectGenerator.INSTANCE,
6566
DropGenerator.INSTANCE,
67+
DropAllGenerator.INSTANCE,
6668
EnrichGenerator.INSTANCE,
6769
EvalGenerator.INSTANCE,
6870
ForkGenerator.INSTANCE,
Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
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.generator.command.pipe;
9+
10+
import org.elasticsearch.xpack.esql.generator.Column;
11+
import org.elasticsearch.xpack.esql.generator.EsqlQueryGenerator;
12+
import org.elasticsearch.xpack.esql.generator.QueryExecutor;
13+
import org.elasticsearch.xpack.esql.generator.command.CommandGenerator;
14+
15+
import java.util.HashSet;
16+
import java.util.List;
17+
import java.util.Map;
18+
import java.util.Set;
19+
import java.util.stream.Collectors;
20+
21+
public class DropAllGenerator implements CommandGenerator {
22+
23+
public static final String DROP_ALL = "drop_all";
24+
public static final String DROPPED_COLUMNS = "dropped_columns";
25+
26+
public static final CommandGenerator INSTANCE = new DropAllGenerator();
27+
28+
@Override
29+
public CommandDescription generate(
30+
List<CommandDescription> previousCommands,
31+
List<Column> previousOutput,
32+
QuerySchema schema,
33+
QueryExecutor executor
34+
) {
35+
Set<String> droppedColumns = new HashSet<>();
36+
String name = EsqlQueryGenerator.randomStringField(previousOutput);
37+
if (name == null || name.isEmpty()) {
38+
return CommandGenerator.EMPTY_DESCRIPTION;
39+
}
40+
41+
String cmdString = " | keep " + name + " | drop " + name;
42+
return new CommandDescription(DROP_ALL, this, cmdString, Map.ofEntries(Map.entry(DROPPED_COLUMNS, droppedColumns)));
43+
}
44+
45+
@Override
46+
@SuppressWarnings("unchecked")
47+
public ValidationResult validateOutput(
48+
List<CommandDescription> previousCommands,
49+
CommandDescription commandDescription,
50+
List<Column> previousColumns,
51+
List<List<Object>> previousOutput,
52+
List<Column> columns,
53+
List<List<Object>> output
54+
) {
55+
if (commandDescription == EMPTY_DESCRIPTION) {
56+
return VALIDATION_OK;
57+
}
58+
59+
if (columns.size() > 0) {
60+
return new ValidationResult(
61+
false,
62+
"Expecting no columns, got [" + columns.stream().map(Column::name).collect(Collectors.joining(", ")) + "]"
63+
);
64+
}
65+
66+
return VALIDATION_OK;
67+
}
68+
69+
}

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

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

5151
dropAllColumns
52-
from employees | keep height | drop height | eval x = 1;
52+
required_capability: fix_no_columns
53+
from employees | limit 4 | keep height | drop height | eval x = 1;
5354

5455
x:integer
56+
1
57+
1
58+
1
59+
1
60+
;
61+
62+
dropEvalKeep
63+
required_capability: fix_no_columns
64+
from employees | drop salary | eval salary = 1 | keep salary | limit 4;
65+
66+
salary:integer
67+
1
68+
1
69+
1
70+
1
5571
;
5672

73+
74+
dropEvalStats
75+
required_capability: fix_no_columns
76+
from mv_sample* | drop `client_ip`, message | eval `event_duration` = "foo", @timestamp = 1 | stats max(@timestamp) by event_duration;
77+
78+
max(@timestamp):integer | event_duration:keyword
79+
1 | foo
80+
;
81+
82+
83+
84+
dropAllColumnsIndexPattern
85+
required_capability: fix_no_columns
86+
from emp* | drop languages | eval languages = 123 | keep languages | limit 4;
87+
88+
languages:integer
89+
123
90+
123
91+
123
92+
123
93+
;
94+
95+
96+
dropAllColumnsWithMetadata
97+
required_capability: fix_no_columns
98+
from employees metadata _index | drop languages | eval languages = 123 | keep languages | limit 4;
99+
100+
languages:integer
101+
123
102+
123
103+
123
104+
123
105+
;
106+
107+
57108
dropAllColumns_WithLimit
109+
required_capability: fix_no_columns
58110
from employees | keep height | drop height | eval x = 1 | limit 3;
59111

60112
x:integer
113+
1
114+
1
115+
1
61116
;
62117

63118
dropAllColumns_WithCount
119+
required_capability: fix_no_columns
64120
from employees | keep height | drop height | eval x = 1 | stats c=count(x);
65121

66122
c:long
67-
0
123+
100
68124
;
69125

70126
dropAllColumns_WithStats
127+
required_capability: fix_no_columns
71128
from employees | keep height | drop height | eval x = 1 | stats c=count(x), mi=min(x), s=sum(x);
72129

73-
c:l|mi:i|s:l
74-
0 |null|null
130+
c:l | mi:i | s:l
131+
100 | 1 | 100
75132
;
76133

77134

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5287,6 +5287,21 @@ null | null | corge | null | null
52875287
null | null | fred | null | null
52885288
;
52895289

5290+
lookupAfterDropAllColumns
5291+
required_capability: fix_no_columns
5292+
required_capability: join_lookup_v12
5293+
FROM languages
5294+
| DROP language_code
5295+
| EVAL language_code = 3
5296+
| LOOKUP JOIN languages_lookup ON language_code
5297+
| LIMIT 3
5298+
;
5299+
5300+
language_code:integer | language_name:keyword
5301+
3 |Spanish
5302+
3 |Spanish
5303+
3 |Spanish
5304+
;
52905305

52915306
lookupJoinWithPushableFilterOnRight
52925307
required_capability: join_lookup_v12

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,3 +246,13 @@ emp_no:integer
246246
10081
247247
// end::sampleForDocs-result[]
248248
;
249+
250+
251+
sampleStatsEval
252+
required_capability: fix_no_columns
253+
required_capability: sample_v3
254+
FROM employees | SAMPLE 0.5 | LIMIT 10 | STATS count = COUNT() | EVAL is_expected = count > 0;
255+
256+
count:long | is_expected:boolean
257+
10 | true
258+
;

0 commit comments

Comments
 (0)