Skip to content

Do not use Min or Max as Top's surrogate when there is an outputField #138380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 24, 2025
Merged

Do not use Min or Max as Top's surrogate when there is an outputField #138380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
954c27f
Do not use Min or Max as Top's surrogate when there is an outputField
ncordon Nov 20, 2025
da1b2e2
Update docs/changelog/138380.yaml
ncordon Nov 20, 2025
672069f
Adds tests for TOP surrogation
ncordon Nov 21, 2025
a8dffb8
Gates test with esql capability
ncordon Nov 21, 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
6 changes: 6 additions & 0 deletions docs/changelog/138380.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 138380
summary: Do not use Min or Max as Top's surrogate when there is an `outputField`
area: ES|QL
type: bug
issues:
- 134083
26 changes: 26 additions & 0 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats_top.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,3 +405,29 @@ birth_year:long | v:integer
1965 | 10095
null | null
;

outputFieldAndShouldSurrogate
required_capability: agg_top_with_output_field

FROM employees
| STATS
min_height = TOP(height, 1, "asc"),
max_height = TOP(height, 1, "desc")
;

min_height:double | max_height:double
1.41 | 2.1
;

outputFieldAndShouldNotSurrogate
required_capability: fix_agg_top_with_output_field_surrogate

FROM employees
| STATS
shortest_employee = TOP(height, 1, "asc", emp_no),
tallest_employee = TOP(height, 1, "desc", emp_no)
;

shortest_employee:integer | tallest_employee:integer
10020 | 10094
;
5 changes: 5 additions & 0 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,11 @@ public enum Cap {
*/
AGG_TOP_WITH_OUTPUT_FIELD,

/**
* Fix for a bug when surrogating a {@code TOP} with limit 1 and output field.
*/
FIX_AGG_TOP_WITH_OUTPUT_FIELD_SURROGATE,

/**
* {@code CASE} properly handling multivalue conditions.
*/
Expand Down
3 changes: 2 additions & 1 deletion ...in/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Top.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,8 @@ public Expression surrogate() {
if (outputField() != null && field().semanticEquals(outputField())) {
return new Top(s, field(), limitField(), orderField(), null);
}
if (orderField() instanceof Literal && limitField() instanceof Literal && limitValue() == 1) {
// To replace Top by Min or Max, we cannot have an `outputField`
if (orderField() instanceof Literal && limitField() instanceof Literal && limitValue() == 1 && outputField() == null) {
Copy link
Contributor

@ivancea ivancea Nov 20, 2025

Choose a reason for hiding this comment

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

Can we add some CSV test reproducing this case? To ensure that this indeed failed before the fix.
A surrogate changing the result type of the expression is a quite hardcore error at planning time.
Our unit tests happen to execute the surrogate logic, but it's really executed as part of the planning, so having an integration test checking it looks safer (Apart of the unit tests, which look fine)

Copy link
Contributor Author

@ncordon ncordon Nov 21, 2025

Choose a reason for hiding this comment

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

Done. If I backtrack the fix, the following error is thrown in those tests (which I find not very helpful), so you are right the planner would not let this through:

Output has changed from [[shortest_employee{r}#577, tallest_employee{r}#581]] to [[shortest_employee{r}#577, tallest_employee{r}#581]].

if (orderValue()) {
return new Min(s, field(), filter(), window());
} else {
Expand Down
62 changes: 59 additions & 3 deletions ...ql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/TopTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,63 @@ public static Iterable<Object[]> parameters() {
equalTo(new BytesRef(InetAddressPoint.encode(InetAddresses.forString("ffff::"))))
)
),

// For cases where we have limit == 1 and an outputField, we should not use the surrogate
new TestCaseSupplier(
List.of(DataType.DOUBLE, DataType.INTEGER, DataType.KEYWORD, DataType.INTEGER),
() -> new TestCaseSupplier.TestCase(
List.of(
TestCaseSupplier.TypedData.multiRow(List.of(3.14, 2.71, 1.41), DataType.DOUBLE, "field"),
new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),
new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral(),
TestCaseSupplier.TypedData.multiRow(List.of(1, 2, 3), DataType.INTEGER, "outputField")
),
"TopDoubleInt",
DataType.INTEGER,
equalTo(1)
)
),
new TestCaseSupplier(
List.of(DataType.DOUBLE, DataType.INTEGER, DataType.KEYWORD, DataType.INTEGER),
() -> new TestCaseSupplier.TestCase(
List.of(
TestCaseSupplier.TypedData.multiRow(List.of(3.14, 2.71, 1.41), DataType.DOUBLE, "field"),
new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),
new TestCaseSupplier.TypedData(new BytesRef("asc"), DataType.KEYWORD, "order").forceLiteral(),
TestCaseSupplier.TypedData.multiRow(List.of(1, 2, 3), DataType.INTEGER, "outputField")
),
"TopDoubleInt",
DataType.INTEGER,
equalTo(3)
)
),
new TestCaseSupplier(
List.of(DataType.INTEGER, DataType.INTEGER, DataType.KEYWORD, DataType.DOUBLE),
() -> new TestCaseSupplier.TestCase(
List.of(
TestCaseSupplier.TypedData.multiRow(List.of(1, 2, 3), DataType.INTEGER, "field"),
new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),
new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral(),
TestCaseSupplier.TypedData.multiRow(List.of(3.14, 2.71, 1.41), DataType.DOUBLE, "outputField")
),
"TopIntDouble",
DataType.DOUBLE,
equalTo(1.41)
)
),
new TestCaseSupplier(
List.of(DataType.INTEGER, DataType.INTEGER, DataType.KEYWORD, DataType.DOUBLE),
() -> new TestCaseSupplier.TestCase(
List.of(
TestCaseSupplier.TypedData.multiRow(List.of(1, 2, 3), DataType.INTEGER, "field"),
new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),
new TestCaseSupplier.TypedData(new BytesRef("asc"), DataType.KEYWORD, "order").forceLiteral(),
TestCaseSupplier.TypedData.multiRow(List.of(3.14, 2.71, 1.41), DataType.DOUBLE, "outputField")
),
"TopIntDouble",
DataType.DOUBLE,
equalTo(3.14)
)
),
// Folding
new TestCaseSupplier(
List.of(DataType.BOOLEAN, DataType.INTEGER, DataType.KEYWORD),
Expand Down Expand Up @@ -455,10 +511,10 @@ private static TestCaseSupplier makeSupplier(
.toList();

String baseName;
if (limit != 1) {
if (limit != 1 || outputFieldSupplied) {
baseName = "Top";
} else {
// If the limit is 1 we rewrite TOP into MIN or MAX and never run our lovely TOP code.
// If the limit is 1 and there is no `outputField` we rewrite TOP into MIN or MAX and never run our lovely TOP code.
if (isAscending) {
baseName = "Min";
} else {
Expand Down