SQL: Enhance message for PERCENTILE[_RANK] with field as 2nd arg (#36933)

Enhance error message for the case that the 2nd argument of PERCENTILE
and PERCENTILE_RANK is not a foldable, as it doesn't make sense to have
a dynamic value coming from a field.

Fixes: #36903

Author: matriv

docs/reference/sql/functions/aggs.asciidoc

@@ -192,7 +192,7 @@ PERCENTILE(field_name<1>, numeric_exp<2>)
 *Input*:
 
 <1> a numeric field
-<2> a numeric expression
+<2> a numeric expression (must be a constant and not based on a field)
 
 *Output*: `double` numeric value
 
@@ -218,7 +218,7 @@ PERCENTILE_RANK(field_name<1>, numeric_exp<2>)
 *Input*:
 
 <1> a numeric field
-<2> a numeric expression
+<2> a numeric expression (must be a constant and not based on a field)
 
 *Output*: `double` numeric value
 

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Percentile.java

@@ -5,6 +5,7 @@
  */
 package org.elasticsearch.xpack.sql.expression.function.aggregate;
 
+import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
 import org.elasticsearch.xpack.sql.expression.Expression;
 import org.elasticsearch.xpack.sql.expression.Expressions;
 import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal;
@@ -41,13 +42,17 @@ public Percentile replaceChildren(List<Expression> newChildren) {
 
     @Override
     protected TypeResolution resolveType() {
-        TypeResolution resolution = super.resolveType();
+        if (!percent.foldable()) {
+            throw new SqlIllegalArgumentException("2nd argument of PERCENTILE must be constant, received [{}]",
+                Expressions.name(percent));
+        }
 
-        if (TypeResolution.TYPE_RESOLVED.equals(resolution)) {
-            resolution = Expressions.typeMustBeNumeric(percent(), functionName(), ParamOrdinal.DEFAULT);
+        TypeResolution resolution = super.resolveType();
+        if (resolution.unresolved()) {
+            return resolution;
         }
 
-        return resolution;
+        return Expressions.typeMustBeNumeric(percent, functionName(), ParamOrdinal.DEFAULT);
     }
 
     public Expression percent() {

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/PercentileRank.java

@@ -5,6 +5,7 @@
  */
 package org.elasticsearch.xpack.sql.expression.function.aggregate;
 
+import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
 import org.elasticsearch.xpack.sql.expression.Expression;
 import org.elasticsearch.xpack.sql.expression.Expressions;
 import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal;
@@ -41,6 +42,11 @@ public Expression replaceChildren(List<Expression> newChildren) {
 
     @Override
     protected TypeResolution resolveType() {
+        if (!value.foldable()) {
+            throw new SqlIllegalArgumentException("2nd argument of PERCENTILE_RANK must be constant, received [{}]",
+                Expressions.name(value));
+        }
+
         TypeResolution resolution = super.resolveType();
         if (resolution.unresolved()) {
             return resolution;

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java

@@ -6,6 +6,7 @@
 package org.elasticsearch.xpack.sql.analysis.analyzer;
 
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
 import org.elasticsearch.xpack.sql.TestUtils;
 import org.elasticsearch.xpack.sql.analysis.AnalysisException;
 import org.elasticsearch.xpack.sql.analysis.index.EsIndex;
@@ -26,12 +27,13 @@
 import java.util.Map;
 
 public class VerifierErrorMessagesTests extends ESTestCase {
+
     private SqlParser parser = new SqlParser();
+    private IndexResolution indexResolution = IndexResolution.valid(new EsIndex("test",
+        TypesTests.loadMapping("mapping-multi-field-with-nested.json")));
 
     private String error(String sql) {
-        Map<String, EsField> mapping = TypesTests.loadMapping("mapping-multi-field-with-nested.json");
-        EsIndex test = new EsIndex("test", mapping);
-        return error(IndexResolution.valid(test), sql);
+        return error(indexResolution, sql);
     }
 
     private String error(IndexResolution getIndexResult, String sql) {
@@ -504,4 +506,20 @@ public void testAggsInHistogram() {
         assertEquals("1:47: Cannot use an aggregate [MAX] for grouping",
                 error("SELECT MAX(date) FROM test GROUP BY HISTOGRAM(MAX(int), 1)"));
     }
-}
+
+    public void testErrorMessageForPercentileWithSecondArgBasedOnAField() {
+        Analyzer analyzer = new Analyzer(TestUtils.TEST_CFG, new FunctionRegistry(), indexResolution, new Verifier(new Metrics()));
+        SqlIllegalArgumentException e = expectThrows(SqlIllegalArgumentException.class, () -> analyzer.analyze(parser.createStatement(
+            "SELECT PERCENTILE(int, ABS(int)) FROM test"), true));
+        assertEquals("2nd argument of PERCENTILE must be constant, received [ABS(int)]",
+            e.getMessage());
+    }
+
+    public void testErrorMessageForPercentileRankWithSecondArgBasedOnAField() {
+        Analyzer analyzer = new Analyzer(TestUtils.TEST_CFG, new FunctionRegistry(), indexResolution, new Verifier(new Metrics()));
+        SqlIllegalArgumentException e = expectThrows(SqlIllegalArgumentException.class, () -> analyzer.analyze(parser.createStatement(
+            "SELECT PERCENTILE_RANK(int, ABS(int)) FROM test"), true));
+        assertEquals("2nd argument of PERCENTILE_RANK must be constant, received [ABS(int)]",
+            e.getMessage());
+    }
+}