Skip to content

Commit 3b4d53e

Browse files
ncordonncordon
authored
Do not use Min or Max as Top's surrogate when there is an outputField (#138380)
1 parent 1de8756 commit 3b4d53e

File tree

5 files changed

+98
-4
lines changed

5 files changed

+98
-4
lines changed

‎docs/changelog/138380.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 138380
2+
summary: Do not use Min or Max as Top's surrogate when there is an `outputField`
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 134083

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,3 +405,29 @@ birth_year:long | v:integer
405405
1965 | 10095
406406
null | null
407407
;
408+
409+
outputFieldAndShouldSurrogate
410+
required_capability: agg_top_with_output_field
411+
412+
FROM employees
413+
| STATS
414+
min_height = TOP(height, 1, "asc"),
415+
max_height = TOP(height, 1, "desc")
416+
;
417+
418+
min_height:double | max_height:double
419+
1.41 | 2.1
420+
;
421+
422+
outputFieldAndShouldNotSurrogate
423+
required_capability: fix_agg_top_with_output_field_surrogate
424+
425+
FROM employees
426+
| STATS
427+
shortest_employee = TOP(height, 1, "asc", emp_no),
428+
tallest_employee = TOP(height, 1, "desc", emp_no)
429+
;
430+
431+
shortest_employee:integer | tallest_employee:integer
432+
10020 | 10094
433+
;

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,11 @@ public enum Cap {
319319
*/
320320
AGG_TOP_WITH_OUTPUT_FIELD,
321321

322+
/**
323+
* Fix for a bug when surrogating a {@code TOP} with limit 1 and output field.
324+
*/
325+
FIX_AGG_TOP_WITH_OUTPUT_FIELD_SURROGATE,
326+
322327
/**
323328
* {@code CASE} properly handling multivalue conditions.
324329
*/

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,8 @@ public Expression surrogate() {
388388
if (outputField() != null && field().semanticEquals(outputField())) {
389389
return new Top(s, field(), limitField(), orderField(), null);
390390
}
391-
if (orderField() instanceof Literal && limitField() instanceof Literal && limitValue() == 1) {
391+
// To replace Top by Min or Max, we cannot have an `outputField`
392+
if (orderField() instanceof Literal && limitField() instanceof Literal && limitValue() == 1 && outputField() == null) {
392393
if (orderValue()) {
393394
return new Min(s, field(), filter(), window());
394395
} else {

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

Lines changed: 59 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,63 @@ public static Iterable<Object[]> parameters() {
176176
equalTo(new BytesRef(InetAddressPoint.encode(InetAddresses.forString("ffff::"))))
177177
)
178178
),
179-
179+
// For cases where we have limit == 1 and an outputField, we should not use the surrogate
180+
new TestCaseSupplier(
181+
List.of(DataType.DOUBLE, DataType.INTEGER, DataType.KEYWORD, DataType.INTEGER),
182+
() -> new TestCaseSupplier.TestCase(
183+
List.of(
184+
TestCaseSupplier.TypedData.multiRow(List.of(3.14, 2.71, 1.41), DataType.DOUBLE, "field"),
185+
new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),
186+
new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral(),
187+
TestCaseSupplier.TypedData.multiRow(List.of(1, 2, 3), DataType.INTEGER, "outputField")
188+
),
189+
"TopDoubleInt",
190+
DataType.INTEGER,
191+
equalTo(1)
192+
)
193+
),
194+
new TestCaseSupplier(
195+
List.of(DataType.DOUBLE, DataType.INTEGER, DataType.KEYWORD, DataType.INTEGER),
196+
() -> new TestCaseSupplier.TestCase(
197+
List.of(
198+
TestCaseSupplier.TypedData.multiRow(List.of(3.14, 2.71, 1.41), DataType.DOUBLE, "field"),
199+
new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),
200+
new TestCaseSupplier.TypedData(new BytesRef("asc"), DataType.KEYWORD, "order").forceLiteral(),
201+
TestCaseSupplier.TypedData.multiRow(List.of(1, 2, 3), DataType.INTEGER, "outputField")
202+
),
203+
"TopDoubleInt",
204+
DataType.INTEGER,
205+
equalTo(3)
206+
)
207+
),
208+
new TestCaseSupplier(
209+
List.of(DataType.INTEGER, DataType.INTEGER, DataType.KEYWORD, DataType.DOUBLE),
210+
() -> new TestCaseSupplier.TestCase(
211+
List.of(
212+
TestCaseSupplier.TypedData.multiRow(List.of(1, 2, 3), DataType.INTEGER, "field"),
213+
new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),
214+
new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral(),
215+
TestCaseSupplier.TypedData.multiRow(List.of(3.14, 2.71, 1.41), DataType.DOUBLE, "outputField")
216+
),
217+
"TopIntDouble",
218+
DataType.DOUBLE,
219+
equalTo(1.41)
220+
)
221+
),
222+
new TestCaseSupplier(
223+
List.of(DataType.INTEGER, DataType.INTEGER, DataType.KEYWORD, DataType.DOUBLE),
224+
() -> new TestCaseSupplier.TestCase(
225+
List.of(
226+
TestCaseSupplier.TypedData.multiRow(List.of(1, 2, 3), DataType.INTEGER, "field"),
227+
new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),
228+
new TestCaseSupplier.TypedData(new BytesRef("asc"), DataType.KEYWORD, "order").forceLiteral(),
229+
TestCaseSupplier.TypedData.multiRow(List.of(3.14, 2.71, 1.41), DataType.DOUBLE, "outputField")
230+
),
231+
"TopIntDouble",
232+
DataType.DOUBLE,
233+
equalTo(3.14)
234+
)
235+
),
180236
// Folding
181237
new TestCaseSupplier(
182238
List.of(DataType.BOOLEAN, DataType.INTEGER, DataType.KEYWORD),
@@ -455,10 +511,10 @@ private static TestCaseSupplier makeSupplier(
455511
.toList();
456512

457513
String baseName;
458-
if (limit != 1) {
514+
if (limit != 1 || outputFieldSupplied) {
459515
baseName = "Top";
460516
} else {
461-
// If the limit is 1 we rewrite TOP into MIN or MAX and never run our lovely TOP code.
517+
// If the limit is 1 and there is no `outputField` we rewrite TOP into MIN or MAX and never run our lovely TOP code.
462518
if (isAscending) {
463519
baseName = "Min";
464520
} else {

0 commit comments

Comments
 (0)