Skip to content

Commit b3a9dae

Browse files
kanoshiouomricohenn
kanoshiou
authored and
omricohenn
committed
ESQL: Fail in AggregateFunction when LogicPlan is not an Aggregate (elastic#124446)
1 parent 9596cdb commit b3a9dae

File tree

4 files changed

+72
-18
lines changed

4 files changed

+72
-18
lines changed

‎docs/changelog/124446.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 124446
2+
summary: "ESQL: Fail in `AggregateFunction` when `LogicPlan` is not an `Aggregate`"
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 124311

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AggregateFunction.java‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,9 @@
1919
import org.elasticsearch.xpack.esql.core.tree.Source;
2020
import org.elasticsearch.xpack.esql.core.util.CollectionUtils;
2121
import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput;
22+
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
23+
import org.elasticsearch.xpack.esql.plan.logical.Dedup;
2224
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
23-
import org.elasticsearch.xpack.esql.plan.logical.OrderBy;
2425

2526
import java.io.IOException;
2627
import java.util.List;
@@ -139,14 +140,12 @@ public boolean equals(Object obj) {
139140
@Override
140141
public BiConsumer<LogicalPlan, Failures> postAnalysisPlanVerification() {
141142
return (p, failures) -> {
142-
if (p instanceof OrderBy order) {
143-
order.order().forEach(o -> {
144-
o.forEachDown(Function.class, f -> {
145-
if (f instanceof AggregateFunction) {
146-
failures.add(fail(f, "Aggregate functions are not allowed in SORT [{}]", f.functionName()));
147-
}
148-
});
149-
});
143+
// `dedup` for now is not exposed as a command,
144+
// so allowing aggregate functions for dedup explicitly is just an internal implementation detail
145+
if ((p instanceof Aggregate) == false && (p instanceof Dedup) == false) {
146+
p.expressions().forEach(x -> x.forEachDown(AggregateFunction.class, af -> {
147+
failures.add(fail(af, "aggregate function [{}] not allowed outside STATS command", af.sourceText()));
148+
}));
150149
}
151150
};
152151
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Eval.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
2525
import org.elasticsearch.xpack.esql.core.tree.Source;
2626
import org.elasticsearch.xpack.esql.core.type.DataType;
27-
import org.elasticsearch.xpack.esql.expression.function.aggregate.AggregateFunction;
2827
import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput;
2928
import org.elasticsearch.xpack.esql.plan.GeneratingPlan;
3029

@@ -178,10 +177,6 @@ public void postAnalysisVerification(Failures failures) {
178177
)
179178
);
180179
}
181-
// check no aggregate functions are used
182-
field.forEachDown(AggregateFunction.class, af -> {
183-
failures.add(fail(af, "aggregate function [{}] not allowed outside STATS command", af.sourceText()));
184-
});
185180
});
186181
}
187182
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java‎

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,6 +1014,17 @@ public void testNotFoundFieldInNestedFunction() {
10141014
line 1:23: Unknown column [avg]""", error("from test | stats c = avg by missing + 1, not_found"));
10151015
}
10161016

1017+
public void testMultipleAggsOutsideStats() {
1018+
assertEquals(
1019+
"""
1020+
1:71: aggregate function [avg(salary)] not allowed outside STATS command
1021+
line 1:96: aggregate function [median(emp_no)] not allowed outside STATS command
1022+
line 1:22: aggregate function [sum(salary)] not allowed outside STATS command
1023+
line 1:39: aggregate function [avg(languages)] not allowed outside STATS command""",
1024+
error("from test | eval s = sum(salary), l = avg(languages) | where salary > avg(salary) and emp_no > median(emp_no)")
1025+
);
1026+
}
1027+
10171028
public void testSpatialSort() {
10181029
String prefix = "ROW wkt = [\"POINT(42.9711 -14.7553)\", \"POINT(75.8093 22.7277)\"] | MV_EXPAND wkt ";
10191030
assertEquals("1:130: cannot sort on geo_point", error(prefix + "| EVAL shape = TO_GEOPOINT(wkt) | limit 5 | sort shape"));
@@ -2107,10 +2118,53 @@ public void testChangePoint_valueNumeric() {
21072118
}
21082119

21092120
public void testSortByAggregate() {
2110-
assertEquals("1:18: Aggregate functions are not allowed in SORT [COUNT]", error("ROW a = 1 | SORT count(*)"));
2111-
assertEquals("1:28: Aggregate functions are not allowed in SORT [COUNT]", error("ROW a = 1 | SORT to_string(count(*))"));
2112-
assertEquals("1:22: Aggregate functions are not allowed in SORT [MAX]", error("ROW a = 1 | SORT 1 + max(a)"));
2113-
assertEquals("1:18: Aggregate functions are not allowed in SORT [COUNT]", error("FROM test | SORT count(*)"));
2121+
assertEquals("1:18: aggregate function [count(*)] not allowed outside STATS command", error("ROW a = 1 | SORT count(*)"));
2122+
assertEquals(
2123+
"1:28: aggregate function [count(*)] not allowed outside STATS command",
2124+
error("ROW a = 1 | SORT to_string(count(*))")
2125+
);
2126+
assertEquals("1:22: aggregate function [max(a)] not allowed outside STATS command", error("ROW a = 1 | SORT 1 + max(a)"));
2127+
assertEquals("1:18: aggregate function [count(*)] not allowed outside STATS command", error("FROM test | SORT count(*)"));
2128+
}
2129+
2130+
public void testFilterByAggregate() {
2131+
assertEquals("1:19: aggregate function [count(*)] not allowed outside STATS command", error("ROW a = 1 | WHERE count(*) > 0"));
2132+
assertEquals(
2133+
"1:29: aggregate function [count(*)] not allowed outside STATS command",
2134+
error("ROW a = 1 | WHERE to_string(count(*)) IS NOT NULL")
2135+
);
2136+
assertEquals("1:23: aggregate function [max(a)] not allowed outside STATS command", error("ROW a = 1 | WHERE 1 + max(a) > 0"));
2137+
assertEquals(
2138+
"1:24: aggregate function [min(languages)] not allowed outside STATS command",
2139+
error("FROM employees | WHERE min(languages) > 2")
2140+
);
2141+
}
2142+
2143+
public void testDissectByAggregate() {
2144+
assertEquals(
2145+
"1:21: aggregate function [min(first_name)] not allowed outside STATS command",
2146+
error("from test | dissect min(first_name) \"%{foo}\"")
2147+
);
2148+
assertEquals(
2149+
"1:21: aggregate function [avg(salary)] not allowed outside STATS command",
2150+
error("from test | dissect avg(salary) \"%{foo}\"")
2151+
);
2152+
}
2153+
2154+
public void testGrokByAggregate() {
2155+
assertEquals(
2156+
"1:18: aggregate function [max(last_name)] not allowed outside STATS command",
2157+
error("from test | grok max(last_name) \"%{WORD:foo}\"")
2158+
);
2159+
assertEquals(
2160+
"1:18: aggregate function [sum(salary)] not allowed outside STATS command",
2161+
error("from test | grok sum(salary) \"%{WORD:foo}\"")
2162+
);
2163+
}
2164+
2165+
public void testAggregateInRow() {
2166+
assertEquals("1:13: aggregate function [count(*)] not allowed outside STATS command", error("ROW a = 1 + count(*)"));
2167+
assertEquals("1:9: aggregate function [avg(2)] not allowed outside STATS command", error("ROW a = avg(2)"));
21142168
}
21152169

21162170
public void testLookupJoinDataTypeMismatch() {

0 commit comments

Comments
 (0)