Fix for top

julian-elastic · julian-elastic · commit 0f080a51870f · 2025-07-10T13:45:17.000-04:00
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/FunctionUtils.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/FunctionUtils.java
@@ -0,0 +1,65 @@
+/*
+ * 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.expression.function;
+
+import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
+import org.elasticsearch.xpack.esql.common.Failures;
+import org.elasticsearch.xpack.esql.core.expression.Expression;
+import org.elasticsearch.xpack.esql.core.expression.Literal;
+
+import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
+import static org.elasticsearch.xpack.esql.common.Failure.fail;
+
+public class FunctionUtils {
+    public static Integer limitValue(Expression limitField, String sourceText) {
+        if (limitField instanceof Literal literal) {
+            Object value = literal.value();
+            if (value instanceof Integer intValue) {
+                return intValue;
+            }
+        }
+        throw new EsqlIllegalArgumentException(format(null, "Limit value must be an integer in [{}], found [{}]", sourceText, limitField));
+    }
+
+    /**
+     * We check that the limit is not null and that if it is a literal, it is a positive integer
+     * We will do a more thorough check in the postOptimizationVerification once folding is done.
+     */
+    public static Expression.TypeResolution resolveTypeLimit(Expression limitField, String sourceText) {
+        if (limitField == null) {
+            return new Expression.TypeResolution(format(null, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField));
+        }
+        if (limitField instanceof Literal literal) {
+            if (literal.value() == null) {
+                return new Expression.TypeResolution(
+                    format(null, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField)
+                );
+            }
+            int value = (Integer) literal.value();
+            if (value <= 0) {
+                return new Expression.TypeResolution(format(null, "Limit must be greater than 0 in [{}], found [{}]", sourceText, value));
+            }
+        }
+        return Expression.TypeResolution.TYPE_RESOLVED;
+    }
+    public static void postOptimizationVerificationLimit(Failures failures, Expression limitField, String sourceText) {
+        if (limitField == null) {
+            failures.add(fail(limitField, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField));
+        }
+        if (limitField instanceof Literal literal) {
+            int value = (Integer) literal.value();
+            if (value <= 0) {
+                failures.add(fail(limitField, "Limit must be greater than 0 in [{}], found [{}]", sourceText, value));
+            }
+        } else {
+            // it is expected that the expression is a literal after folding
+            // we fail if it is not a literal
+            failures.add(fail(limitField, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField));
+        }
+    }
+
+}
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Top.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Top.java
@@ -20,8 +20,9 @@
 import org.elasticsearch.compute.aggregation.TopIpAggregatorFunctionSupplier;
 import org.elasticsearch.compute.aggregation.TopLongAggregatorFunctionSupplier;
 import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
+import org.elasticsearch.xpack.esql.capabilities.PostOptimizationVerificationAware;
+import org.elasticsearch.xpack.esql.common.Failures;
 import org.elasticsearch.xpack.esql.core.expression.Expression;
-import org.elasticsearch.xpack.esql.core.expression.FoldContext;
 import org.elasticsearch.xpack.esql.core.expression.Literal;
 import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
 import org.elasticsearch.xpack.esql.core.tree.Source;
@@ -30,6 +31,7 @@
 import org.elasticsearch.xpack.esql.expression.function.Example;
 import org.elasticsearch.xpack.esql.expression.function.FunctionInfo;
 import org.elasticsearch.xpack.esql.expression.function.FunctionType;
+import org.elasticsearch.xpack.esql.expression.function.FunctionUtils;
 import org.elasticsearch.xpack.esql.expression.function.Param;
 import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput;
 import org.elasticsearch.xpack.esql.planner.ToAggregator;
@@ -39,14 +41,15 @@
 
 import static java.util.Arrays.asList;
 import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
+import static org.elasticsearch.xpack.esql.common.Failure.fail;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.THIRD;
-import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isNotNullAndFoldable;
+import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isNotNull;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isString;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isType;
 
-public class Top extends AggregateFunction implements ToAggregator, SurrogateExpression {
+public class Top extends AggregateFunction implements ToAggregator, SurrogateExpression, PostOptimizationVerificationAware {
     public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(Expression.class, "Top", Top::new);
 
     private static final String ORDER_ASC = "ASC";
@@ -116,16 +119,18 @@ Expression orderField() {
         return parameters().get(1);
     }
 
-    private int limitValue() {
-        return (int) limitField().fold(FoldContext.small() /* TODO remove me */);
-    }
-
-    private String orderRawValue() {
-        return BytesRefs.toString(orderField().fold(FoldContext.small() /* TODO remove me */));
+    private Integer limitValue() {
+        return FunctionUtils.limitValue(limitField(), sourceText());
     }
 
     private boolean orderValue() {
-        return orderRawValue().equalsIgnoreCase(ORDER_ASC);
+        if (orderField() instanceof Literal literal) {
+            String order = BytesRefs.toString(literal.value());
+            if (ORDER_ASC.equalsIgnoreCase(order) || ORDER_DESC.equalsIgnoreCase(order)) {
+                return order.equalsIgnoreCase(ORDER_ASC);
+            }
+        }
+        throw new EsqlIllegalArgumentException("Order value must be a literal, found: " + orderField());
     }
 
     @Override
@@ -148,31 +153,88 @@ protected TypeResolution resolveType() {
             "ip",
             "string",
             "numeric except unsigned_long or counter types"
-        ).and(isNotNullAndFoldable(limitField(), sourceText(), SECOND))
+        ).and(isNotNull(limitField(), sourceText(), SECOND))
             .and(isType(limitField(), dt -> dt == DataType.INTEGER, sourceText(), SECOND, "integer"))
-            .and(isNotNullAndFoldable(orderField(), sourceText(), THIRD))
+            .and(isNotNull(orderField(), sourceText(), THIRD))
             .and(isString(orderField(), sourceText(), THIRD));
 
         if (typeResolution.unresolved()) {
             return typeResolution;
         }
 
-        var limit = limitValue();
-        var order = orderRawValue();
-
-        if (limit <= 0) {
-            return new TypeResolution(format(null, "Limit must be greater than 0 in [{}], found [{}]", sourceText(), limit));
+        TypeResolution result = resolveTypeLimit();
+        if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
+            return result;
         }
-
-        if (order.equalsIgnoreCase(ORDER_ASC) == false && order.equalsIgnoreCase(ORDER_DESC) == false) {
-            return new TypeResolution(
-                format(null, "Invalid order value in [{}], expected [{}, {}] but got [{}]", sourceText(), ORDER_ASC, ORDER_DESC, order)
-            );
+        result = resolveTypeOrder();
+        if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
+            return result;
         }
+        return TypeResolution.TYPE_RESOLVED;
+    }
+
+    /**
+     * We check that the limit is not null and that if it is a literal, it is a positive integer
+     * We will do a more thorough check in the postOptimizationVerification once folding is done.
+     */
+    private TypeResolution resolveTypeLimit() {
+        return FunctionUtils.resolveTypeLimit(limitField(), sourceText());
+    }
 
+    /**
+     * We check that the order is not null and that if it is a literal, it is one of the two valid values: "asc" or "desc".
+     * We will do a more thorough check in the postOptimizationVerification once folding is done.
+     */
+    private TypeResolution resolveTypeOrder() {
+        Expression order = orderField();
+        if (order == null) {
+            return new TypeResolution(format(null, "Order must be a valid string in [{}], found [{}]", sourceText(), order));
+        }
+        if (order instanceof Literal literal) {
+            if (literal.value() == null) {
+                return new TypeResolution(
+                    format(null, "Invalid order value in [{}], expected [{}, {}] but got [{}]", sourceText(), ORDER_ASC, ORDER_DESC, order)
+                );
+            }
+            String value = BytesRefs.toString(literal.value());
+            if (value == null || value.equalsIgnoreCase(ORDER_ASC) == false && value.equalsIgnoreCase(ORDER_DESC) == false) {
+                return new TypeResolution(
+                    format(null, "Invalid order value in [{}], expected [{}, {}] but got [{}]", sourceText(), ORDER_ASC, ORDER_DESC, order)
+                );
+            }
+        }
         return TypeResolution.TYPE_RESOLVED;
     }
 
+    @Override
+    public void postOptimizationVerification(Failures failures) {
+        postOptimizationVerificationLimit(failures);
+        postOptimizationVerificationOrder(failures);
+    }
+
+    private void postOptimizationVerificationLimit(Failures failures) {
+        FunctionUtils.postOptimizationVerificationLimit(failures, limitField(), sourceText());
+    }
+
+    private void postOptimizationVerificationOrder(Failures failures) {
+        Expression order = orderField();
+        if (order == null) {
+            failures.add(fail(order, "Order must be a valid string in [{}], found [{}]", sourceText(), order));
+        }
+        if (order instanceof Literal literal) {
+            String value = BytesRefs.toString(literal.value());
+            if (value == null || value.equalsIgnoreCase(ORDER_ASC) == false && value.equalsIgnoreCase(ORDER_DESC) == false) {
+                failures.add(
+                    fail(order, "Invalid order value in [{}], expected [{}, {}] but got [{}]", sourceText(), ORDER_ASC, ORDER_DESC, order)
+                );
+            }
+        } else {
+            // it is expected that the expression is a literal after folding
+            // we fail if it is not a literal
+            failures.add(fail(order, "Order must be a valid string in [{}], found [{}]", sourceText(), order));
+        }
+    }
+
     @Override
     public DataType dataType() {
         return field().dataType().noText();
@@ -215,15 +277,20 @@ public AggregatorFunctionSupplier supplier() {
     @Override
     public Expression surrogate() {
         var s = source();
-
-        if (limitValue() == 1) {
-            if (orderValue()) {
-                return new Min(s, field());
-            } else {
-                return new Max(s, field());
+        try {
+            if (limitValue() == 1) {
+                if (orderValue()) {
+                    return new Min(s, field());
+                } else {
+                    return new Max(s, field());
+                }
             }
+        } catch (EsqlIllegalArgumentException e) {
+            // If the limit is not a literal or is not a positive integer, we cannot create a surrogate
+            // so we return null to indicate that no surrogate can be created.
+            // This is possible if the limit is an expression, and folding has not been done yet.
+            return null;
         }
-
         return null;
     }
 }
diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/TopTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/TopTests.java
@@ -262,7 +262,7 @@ public static Iterable<Object[]> parameters() {
                             new TestCaseSupplier.TypedData(null, DataType.INTEGER, "limit").forceLiteral(),
                             new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral()
                         ),
-                        "second argument of [source] cannot be null, received [limit]"
+                        "Limit must be a constant integer in [source], found [null]"
                     )
                 ),
                 new TestCaseSupplier(
@@ -273,7 +273,7 @@ public static Iterable<Object[]> parameters() {
                             new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),
                             new TestCaseSupplier.TypedData(null, DataType.KEYWORD, "order").forceLiteral()
                         ),
-                        "third argument of [source] cannot be null, received [order]"
+                        "Invalid order value in [source], expected [ASC, DESC] but got [null]"
                     )
                 )
             )
diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/230_folding.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/230_folding.yml
@@ -0,0 +1,97 @@
+---
+setup:
+  - requires:
+      test_runner_features: [ capabilities ]
+      capabilities:
+        - method: POST
+          path: /_query
+          parameters: [ ]
+          capabilities: [ agg_top ]
+      reason: "uses TOP function in STATS aggregation"
+  - do:
+      indices.create:
+        index: employees
+        body:
+          mappings:
+            properties:
+              hire_date:
+                type: date
+              salary_change:
+                type: double
+              salary:
+                type: integer
+              salary_change_long:
+                type: long
+  - do:
+      bulk:
+        index: employees
+        refresh: true
+        body:
+          - { "index": { } }
+          - { "hire_date": "2020-01-01", "salary_change": 100.5, "salary": 50000, "salary_change_long": 100 }
+          - { "index": { } }
+          - { "hire_date": "2021-01-01", "salary_change": 200.5, "salary": 60000, "salary_change_long": 200 }
+          - { "index": { } }
+          - { "hire_date": "2019-01-01", "salary_change": 50.5, "salary": 40000, "salary_change_long": 50 }
+
+---
+Top function with constant folding:
+  - do:
+      esql.query:
+        body:
+          query: |
+            FROM employees
+            | STATS
+                date = TOP(hire_date, 1+1, "dEsc"),
+                double = TOP(salary_change, 100-98, REVERSE("csed")),
+                integer = TOP(salary, 4-(1+1), Substring("Ascending",0,3)),
+                long = TOP(salary_change_long, 10 - 4*2, Concat("as","c"))
+            | LIMIT 5
+  - match: { columns.0.name: "date" }
+  - match: { columns.1.name: "double" }
+  - match: { columns.2.name: "integer" }
+  - match: { columns.3.name: "long" }
+  - length: { values: 1 }
+  - length: { values.0: 4 }
+  # Check that the values are as expected for the folded constants
+  - match: { values.0.0: [ "2021-01-01T00:00:00.000Z", "2020-01-01T00:00:00.000Z" ] }
+  - match: { values.0.1: [ 200.5, 100.5 ] }
+  - match: { values.0.2: [ 40000, 50000 ] }
+  - match: { values.0.3: [ 50, 100 ] }
+
+
+---
+
+Top function with negative limit value after folding:
+  - do:
+      catch: bad_request
+      esql.query:
+        body:
+          query: |
+            FROM employees
+            | STATS
+                date = TOP(hire_date, 10 - 20, "dEsc"),
+                double = TOP(salary_change, 100-98, REVERSE("csed")),
+                integer = TOP(salary, 4-(1+1), Substring("Ascending",0,3)),
+                long = TOP(salary_change_long, 10 - 4*2, Concat("as","c"))
+            | LIMIT 5
+  - match: { error.type: "verification_exception" }
+  - match: { error.reason: "Found 1 problem\nline 3:27: Limit must be greater than 0 in [TOP(hire_date, 10 - 20, \"dEsc\")], found [-10]" }
+
+---
+
+Top function with invalid sort order:
+  - do:
+      catch: bad_request
+      esql.query:
+        body:
+          query: |
+            FROM employees
+            | STATS
+                date = TOP(hire_date, 2, REVERSE("csed123")),
+                double = TOP(salary_change, 100-98, REVERSE("csed")),
+                integer = TOP(salary, 4-(1+1), Substring("Ascending",0,3)),
+                long = TOP(salary_change_long, 10 - 4*2, Concat("as","c"))
+            | LIMIT 5
+  - match: { error.type: "verification_exception" }
+  - match: { error.reason: "Found 1 problem\nline 3:30: Invalid order value in [TOP(hire_date, 2, REVERSE(\"csed123\"))], expected [ASC, DESC] but got [321desc]" }

Original file line number	Diff line number	Diff line change
`@@ -262,7 +262,7 @@ public static Iterable<Object[]> parameters() {`
`262`	`262`	`new TestCaseSupplier.TypedData(null, DataType.INTEGER, "limit").forceLiteral(),`
`263`	`263`	`new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral()`
`264`	`264`	`),`
`265`		`- "second argument of [source] cannot be null, received [limit]"`
	`265`	`+ "Limit must be a constant integer in [source], found [null]"`
`266`	`266`	`)`
`267`	`267`	`),`
`268`	`268`	`new TestCaseSupplier(`
`@@ -273,7 +273,7 @@ public static Iterable<Object[]> parameters() {`
`273`	`273`	`new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),`
`274`	`274`	`new TestCaseSupplier.TypedData(null, DataType.KEYWORD, "order").forceLiteral()`
`275`	`275`	`),`
`276`		`- "third argument of [source] cannot be null, received [order]"`
	`276`	`+ "Invalid order value in [source], expected [ASC, DESC] but got [null]"`
`277`	`277`	`)`
`278`	`278`	`)`
`279`	`279`	`)`