Skip to content

Commit ffe75ea

Browse files
ESQL: Retain aggregate when grouping (#126598) (#127253)
Co-authored-by: kanoshiou <[email protected]>
1 parent ea99f1a commit ffe75ea

File tree

6 files changed

+121
-9
lines changed

6 files changed

+121
-9
lines changed

docs/changelog/126598.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 126598
2+
summary: "ESQL: Retain aggregate when grouping"
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 126026

x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public abstract class GenerativeRestTest extends ESRestTestCase {
4848
"Plan \\[ProjectExec\\[\\[<no-fields>.* optimized incorrectly due to missing references", // https://github.com/elastic/elasticsearch/issues/125866
4949
"only supports KEYWORD or TEXT values, found expression", // https://github.com/elastic/elasticsearch/issues/126017
5050
"token recognition error at: '``", // https://github.com/elastic/elasticsearch/issues/125870
51-
"Unknown column \\[.*\\]", // https://github.com/elastic/elasticsearch/issues/126026
51+
// https://github.com/elastic/elasticsearch/issues/127167
5252
"optimized incorrectly due to missing references", // https://github.com/elastic/elasticsearch/issues/116781
5353
"No matches found for pattern", // https://github.com/elastic/elasticsearch/issues/126418
5454
"The incoming YAML document exceeds the limit:" // still to investigate, but it seems to be specific to the test framework

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

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -673,3 +673,82 @@ ROW foo = [10, 11, 12]
673673
max:integer
674674
13
675675
;
676+
677+
evalAfterAvgGroupingUsingSameName
678+
required_capability: retain_aggregate_when_grouping
679+
from employees
680+
| stats max = max(salary), avg = avg(salary) by g = gender
681+
| keep max, g
682+
| eval max = 12, g = 3
683+
;
684+
685+
max:integer | g:integer
686+
12 | 3
687+
12 | 3
688+
12 | 3
689+
;
690+
691+
evalAfterGroupingUsingSameName
692+
required_capability: retain_aggregate_when_grouping
693+
row foo = [10,11,9], bar = [1,2,3]
694+
| mv_expand foo
695+
| mv_expand bar
696+
| stats this = max(foo) by bar
697+
| keep this
698+
| eval this = 12
699+
;
700+
701+
this:integer
702+
12
703+
12
704+
12
705+
;
706+
707+
evalAfterGroupingUsingSameName2
708+
required_capability: retain_aggregate_when_grouping
709+
from employees
710+
| stats count = count(emp_no) by gender, is_rehired
711+
| keep count
712+
| rename count as x
713+
| keep x
714+
| eval x = 12
715+
;
716+
717+
x:integer
718+
12
719+
12
720+
12
721+
12
722+
12
723+
12
724+
12
725+
12
726+
12
727+
;
728+
729+
evalAfterGroupingUsingSameName3
730+
required_capability: retain_aggregate_when_grouping
731+
row foo = 10
732+
| stats field_1 = max(foo), this = count(*), field_2 = max(foo) by foo
733+
| rename this AS field_2
734+
| keep field_1, field_2
735+
| eval field_2 = 1, field_1 = 1
736+
;
737+
738+
field_2:integer | field_1:integer
739+
1 | 1
740+
;
741+
742+
evalAfterGroupingUsingSameName4
743+
required_capability: retain_aggregate_when_grouping
744+
from employees
745+
| stats sum = sum(salary), max = max(salary) by g = gender
746+
| keep max, g
747+
| eval g = 12, max = 2
748+
;
749+
750+
g:integer | max:integer
751+
12 | 2
752+
12 | 2
753+
12 | 2
754+
;

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,12 @@ public enum Cap {
283283
*/
284284
REMOVE_EMPTY_ATTRIBUTE_IN_MERGING_OUTPUT,
285285

286+
/**
287+
* Support for retain aggregate when grouping.
288+
* See <a href="https://github.com/elastic/elasticsearch/issues/126026"> ES|QL: columns not projected away despite KEEP #126026 </a>
289+
*/
290+
RETAIN_AGGREGATE_WHEN_GROUPING,
291+
286292
/**
287293
* Fix for union-types when some indexes are missing the required field. Done in #111932.
288294
*/

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010
import org.elasticsearch.compute.data.Block;
1111
import org.elasticsearch.compute.data.BlockUtils;
1212
import org.elasticsearch.index.IndexMode;
13+
import org.elasticsearch.xpack.esql.core.expression.Alias;
14+
import org.elasticsearch.xpack.esql.core.expression.Attribute;
1315
import org.elasticsearch.xpack.esql.core.expression.AttributeSet;
1416
import org.elasticsearch.xpack.esql.core.expression.Expressions;
1517
import org.elasticsearch.xpack.esql.core.expression.NamedExpression;
@@ -71,14 +73,12 @@ public LogicalPlan apply(LogicalPlan plan) {
7173
);
7274
} else {
7375
// Aggs cannot produce pages with 0 columns, so retain one grouping.
74-
remaining = List.of(Expressions.attribute(aggregate.groupings().get(0)));
75-
p = new Aggregate(
76-
aggregate.source(),
77-
aggregate.child(),
78-
aggregate.aggregateType(),
79-
aggregate.groupings(),
80-
remaining
76+
Attribute attribute = Expressions.attribute(aggregate.groupings().get(0));
77+
NamedExpression firstAggregate = aggregate.aggregates().get(0);
78+
remaining = List.of(
79+
new Alias(firstAggregate.source(), firstAggregate.name(), attribute, firstAggregate.id())
8180
);
81+
p = aggregate.with(aggregate.groupings(), remaining);
8282
}
8383
} else {
8484
p = new Aggregate(

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

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,7 @@ public void testEmptyProjectInStatWithGroupAndEval() {
364364
var relation = as(filter.child(), EsRelation.class);
365365

366366
assertThat(Expressions.names(agg.groupings()), contains("emp_no"));
367-
assertThat(Expressions.names(agg.aggregates()), contains("emp_no"));
367+
assertThat(Expressions.names(agg.aggregates()), contains("c"));
368368

369369
var exprs = eval.fields();
370370
assertThat(exprs.size(), equalTo(1));
@@ -2743,6 +2743,27 @@ public void testEvalAfterStats() {
27432743
assertThat(Expressions.names(eval.output()), contains("x"));
27442744
}
27452745

2746+
/**
2747+
* Expects
2748+
* Eval[[2[INTEGER] AS x]]
2749+
* \_Limit[1000[INTEGER],false]
2750+
* \_Aggregate[[foo{r}#3],[foo{r}#3 AS x]]
2751+
* \_LocalRelation[[foo{r}#3],[IntVectorBlock[vector=ConstantIntVector[positions=1, value=1]]]]
2752+
*/
2753+
public void testEvalAfterGroupBy() {
2754+
var plan = optimizedPlan("""
2755+
ROW foo = 1
2756+
| STATS x = max(foo) by foo
2757+
| KEEP x
2758+
| EVAL x = 2
2759+
""");
2760+
var eval = as(plan, Eval.class);
2761+
var limit = as(eval.child(), Limit.class);
2762+
var aggregate = as(limit.child(), Aggregate.class);
2763+
var localRelation = as(aggregate.child(), LocalRelation.class);
2764+
assertThat(Expressions.names(eval.output()), contains("x"));
2765+
}
2766+
27462767
public void testCombineLimitWithOrderByThroughFilterAndEval() {
27472768
LogicalPlan plan = optimizedPlan("""
27482769
from test

0 commit comments

Comments
 (0)