Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
ad961ae
ES|QL:Fix wrong pruning of plans with no output columns
luigidellaquila Aug 22, 2025
21dc299
Fix test
luigidellaquila Aug 22, 2025
de95b79
Update docs/changelog/133405.yaml
luigidellaquila Aug 22, 2025
42d364f
Fix BWC
luigidellaquila Aug 22, 2025
544d6dd
Merge remote-tracking branch 'luigidellaquila/esql/fix_no_columns' in…
luigidellaquila Aug 22, 2025
c2f01d6
Restore original tests
luigidellaquila Aug 25, 2025
ba01afe
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Aug 25, 2025
764cace
Fix tests
luigidellaquila Aug 25, 2025
642b50a
Refactor local suppliers to return a Page
luigidellaquila Aug 25, 2025
3ae2314
Fix flaky test
luigidellaquila Aug 25, 2025
e09cabd
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Aug 26, 2025
a1aaa5b
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Aug 27, 2025
0421de5
More tests
luigidellaquila Aug 28, 2025
34b995a
Tests
luigidellaquila Aug 28, 2025
2e35ab3
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Aug 28, 2025
be75856
More tests
luigidellaquila Aug 28, 2025
657c94e
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Aug 28, 2025
a3cccde
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Aug 28, 2025
dd74c0b
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Sep 8, 2025
afd0350
More tests
luigidellaquila Sep 8, 2025
b45fc9a
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Sep 8, 2025
b42b09a
Fix pushdown stats and new tests
luigidellaquila Sep 8, 2025
3a388a7
BWC
luigidellaquila Sep 8, 2025
076db41
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Sep 9, 2025
b0a9d02
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Sep 9, 2025
130fa2e
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Sep 16, 2025
c0ac934
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Sep 30, 2025
710fdcc
Fix compile and add transport version
luigidellaquila Sep 30, 2025
edc3890
Fix test
luigidellaquila Sep 30, 2025
d2a5631
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Sep 30, 2025
921ecb8
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Sep 30, 2025
59207cb
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Oct 1, 2025
bb45498
Merge remote-tracking branch 'luigidellaquila/esql/fix_no_columns' in…
luigidellaquila Oct 1, 2025
d059447
Merge branch 'main' into esql/fix_no_columns
luigidellaquila Oct 2, 2025
f8ca703
More tests
luigidellaquila Oct 2, 2025
e9ebf8b
Implement suggestions
luigidellaquila Oct 2, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/133405.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 133405
summary: Fix wrong pruning of plans with no output columns
area: ES|QL
type: bug
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,7 @@ static TransportVersion def(int id) {
public static final TransportVersion DATA_STREAM_WRITE_INDEX_ONLY_SETTINGS = def(9_142_0_00);
public static final TransportVersion SCRIPT_RESCORER = def(9_143_0_00);
public static final TransportVersion ESQL_LOOKUP_OPERATOR_EMITTED_ROWS = def(9_144_0_00);
public static final TransportVersion ESQL_PLAN_WITH_NO_COLUMNS = def(9_145_0_00);

/*
* STOP! READ THIS FIRST! No, really,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.elasticsearch.compute.data.DoubleBlock;
import org.elasticsearch.compute.data.IntBlock;
import org.elasticsearch.compute.data.LongBlock;
import org.elasticsearch.compute.data.Page;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.core.Tuple;
Expand Down Expand Up @@ -475,7 +476,11 @@ public static LogicalPlan emptySource() {
}

public static LogicalPlan localSource(BlockFactory blockFactory, List<Attribute> fields, List<Object> row) {
return new LocalRelation(Source.EMPTY, fields, LocalSupplier.of(BlockUtils.fromListRow(blockFactory, row)));
return new LocalRelation(
Source.EMPTY,
fields,
LocalSupplier.of(row.isEmpty() ? new Page(0) : new Page(BlockUtils.fromListRow(blockFactory, row)))
);
}

public static <T> T as(Object node, Class<T> type) {
Expand Down
113 changes: 110 additions & 3 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/drop.csv-spec
Original file line number Diff line number Diff line change
Expand Up @@ -49,29 +49,136 @@ b:integer | x:integer
;

dropAllColumns
required_capability: fix_no_columns
from employees | keep height | drop height | eval x = 1;

x:integer
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
;

dropAllColumns_WithLimit
required_capability: fix_no_columns
from employees | keep height | drop height | eval x = 1 | limit 3;

x:integer
1
1
1
;

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

c:long
0
100
;

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

c:l|mi:i|s:l
0 |null|null
c:l | mi:i | s:l
100 | 1 | 100
;


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ public void testLookupJoinEmptyIndex() throws IOException {
setSkipUnavailable(REMOTE_CLUSTER_1, randomBoolean());

Exception ex;
for (String index : List.of("values_lookup", "values_lookup_map", "values_lookup_map_lookup")) {
for (String index : List.of("values_lookup", "values_lookup_map_lookup")) {
ex = expectThrows(
VerificationException.class,
() -> runQuery("FROM logs-* | LOOKUP JOIN " + index + " ON v | KEEP v", randomBoolean())
Expand All @@ -449,6 +449,29 @@ public void testLookupJoinEmptyIndex() throws IOException {
);
assertThat(ex.getMessage(), containsString("Unknown column [v] in right side of join"));
}

ex = expectThrows(
VerificationException.class,
() -> runQuery("FROM logs-* | LOOKUP JOIN values_lookup_map ON v | KEEP v", randomBoolean())
);
assertThat(
ex.getMessage(),
containsString(
"Lookup Join requires a single lookup mode index; "
+ "[values_lookup_map] resolves to [values_lookup_map] in [standard] mode"
)
);
ex = expectThrows(
VerificationException.class,
() -> runQuery("FROM c*:logs-* | LOOKUP JOIN values_lookup_map ON v | KEEP v", randomBoolean())
);
assertThat(
ex.getMessage(),
containsString(
"Lookup Join requires a single lookup mode index; "
+ "[values_lookup_map] resolves to [cluster-a:values_lookup_map] in [standard] mode"
)
);
}

public void testLookupJoinIndexMode() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -951,15 +951,15 @@ public void testDropAllColumns() {
logger.info(results);
assertThat(results.columns(), hasSize(1));
assertThat(results.columns(), contains(new ColumnInfoImpl("a", "integer", null)));
assertThat(getValuesList(results), is(empty()));
assertThat(getValuesList(results).size(), is(40));
}
}

public void testDropAllColumnsWithStats() {
try (EsqlQueryResponse results = run("from test | stats g = count(data) | drop g")) {
logger.info(results);
assertThat(results.columns(), is(empty()));
assertThat(getValuesList(results), is(empty()));
assertThat(getValuesList(results).size(), is(1));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1389,7 +1389,13 @@ public enum Cap {
/**
* Support for vector Hamming distance.
*/
HAMMING_VECTOR_SIMILARITY_FUNCTION(Build.current().isSnapshot());
HAMMING_VECTOR_SIMILARITY_FUNCTION(Build.current().isSnapshot()),

/**
* Fix management of plans with no columns
* https://github.com/elastic/elasticsearch/issues/120272
*/
FIX_NO_COLUMNS;

private final boolean enabled;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.common.lucene.BytesRefs;
import org.elasticsearch.compute.data.AggregateMetricDoubleBlockBuilder;
import org.elasticsearch.compute.data.Block;
import org.elasticsearch.compute.data.Page;
import org.elasticsearch.core.Strings;
import org.elasticsearch.index.IndexMode;
import org.elasticsearch.logging.Logger;
Expand Down Expand Up @@ -458,7 +459,7 @@ private LocalRelation tableMapAsRelation(Source source, Map<String, Column> mapT
// prepare the block for the supplier
blocks[i++] = column.values();
}
LocalSupplier supplier = LocalSupplier.of(blocks);
LocalSupplier supplier = LocalSupplier.of(blocks.length > 0 ? new Page(blocks) : new Page(0));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if we could integrate the logic "if the blocks size is 0 then new Page(0) otherwise new Page(blocks)" somehow with LocalSupplier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want this in general, in some cases we want no blocks but new Page(N)

return new LocalRelation(source, attributes, supplier);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ public static Tuple<Expression, List<Expression>> extractCommon(List<Expression>
}
splitAnds.add(split);
}
if (common == null) {
common = List.of();
}

List<Expression> trimmed = new ArrayList<>(expressions.size());
final List<Expression> finalCommon = common;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateNullable;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropgateUnmappedFields;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneColumns;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneEmptyPlans;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneEmptyAggregates;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneFilters;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneLiteralsInOrderBy;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneRedundantOrderBy;
Expand Down Expand Up @@ -166,7 +166,6 @@ protected static Batch<LogicalPlan> operators(boolean local) {
"Operator Optimization",
new CombineProjections(local),
new CombineEvals(),
new PruneEmptyPlans(),
new PropagateEmptyRelation(),
new FoldNull(),
new SplitInWithFoldableValue(),
Expand Down Expand Up @@ -203,7 +202,8 @@ protected static Batch<LogicalPlan> operators(boolean local) {
new PushDownAndCombineOrderBy(),
new PruneRedundantOrderBy(),
new PruneRedundantSortClauses(),
new PruneLeftJoinOnNullMatchingField()
new PruneLeftJoinOnNullMatchingField(),
new PruneEmptyAggregates()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this because our aggs don't know how to deal with no grouping and no aggs at the same time.

);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.elasticsearch.compute.data.Block;
import org.elasticsearch.compute.data.BlockFactory;
import org.elasticsearch.compute.data.BlockUtils;
import org.elasticsearch.compute.data.Page;
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
import org.elasticsearch.xpack.esql.core.expression.Alias;
import org.elasticsearch.xpack.esql.core.expression.FoldContext;
Expand Down Expand Up @@ -41,7 +42,10 @@ protected LogicalPlan rule(UnaryPlan plan, LogicalOptimizerContext ctx) {
// only care about non-grouped aggs might return something (count)
if (plan instanceof Aggregate agg && agg.groupings().isEmpty()) {
List<Block> emptyBlocks = aggsFromEmpty(ctx.foldCtx(), agg.aggregates());
p = replacePlanByRelation(plan, LocalSupplier.of(emptyBlocks.toArray(Block[]::new)));
p = replacePlanByRelation(
plan,
LocalSupplier.of(emptyBlocks.isEmpty() ? new Page(0) : new Page(emptyBlocks.toArray(Block[]::new)))
);
} else {
p = PruneEmptyPlans.skipPlan(plan);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@

package org.elasticsearch.xpack.esql.optimizer.rules.logical;

import org.elasticsearch.compute.data.Block;
import org.elasticsearch.compute.data.BlockUtils;
import org.elasticsearch.compute.data.Page;
import org.elasticsearch.index.IndexMode;
import org.elasticsearch.xpack.esql.core.expression.Alias;
import org.elasticsearch.xpack.esql.core.expression.Attribute;
Expand Down Expand Up @@ -111,7 +111,7 @@ private static LogicalPlan pruneColumnsInAggregate(Aggregate aggregate, Attribut
p = new LocalRelation(
aggregate.source(),
List.of(Expressions.attribute(aggregate.aggregates().getFirst())),
LocalSupplier.of(new Block[] { BlockUtils.constantBlock(PlannerUtils.NON_BREAKING_BLOCK_FACTORY, null, 1) })
LocalSupplier.of(new Page(BlockUtils.constantBlock(PlannerUtils.NON_BREAKING_BLOCK_FACTORY, null, 1)))
);
} else {
// Aggs cannot produce pages with 0 columns, so retain one grouping.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.esql.optimizer.rules.logical;

import org.elasticsearch.compute.data.Page;
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier;

import java.util.List;

public final class PruneEmptyAggregates extends OptimizerRules.OptimizerRule<Aggregate> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help to have a javadoc on this rule. I mean, it is obvious what it does, but it would be helpful to read about the situations that lead to an Aggregate with no aggregates and no groupings.

@Override
protected LogicalPlan rule(Aggregate agg) {
if (agg.aggregates().isEmpty() && agg.groupings().isEmpty()) {
return new LocalRelation(agg.source(), List.of(), LocalSupplier.of(new Page(1)));
}
return agg;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
package org.elasticsearch.xpack.esql.optimizer.rules.logical;

import org.elasticsearch.compute.data.BlockUtils;
import org.elasticsearch.compute.data.Page;
import org.elasticsearch.xpack.esql.optimizer.LogicalOptimizerContext;
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
import org.elasticsearch.xpack.esql.plan.logical.Row;
Expand All @@ -29,6 +30,6 @@ protected LogicalPlan rule(Row row, LogicalOptimizerContext context) {
List<Object> values = new ArrayList<>(fields.size());
fields.forEach(f -> values.add(f.child().fold(context.foldCtx())));
var blocks = BlockUtils.fromListRow(PlannerUtils.NON_BREAKING_BLOCK_FACTORY, values);
return new LocalRelation(row.source(), row.output(), new CopyingLocalSupplier(blocks));
return new LocalRelation(row.source(), row.output(), new CopyingLocalSupplier(blocks.length == 0 ? new Page(0) : new Page(blocks)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about this common logic where a Page is built this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

}
}
Loading