diff --git a/docs/changelog/99827.yaml b/docs/changelog/99827.yaml new file mode 100644 index 0000000000000..3e6690a8e9e68 --- /dev/null +++ b/docs/changelog/99827.yaml @@ -0,0 +1,5 @@ +pr: 99827 +summary: "ESQL: Fix NPE when aggregating literals" +area: ES|QL +type: bug +issues: [] diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats_count_distinct.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats_count_distinct.csv-spec index 462045d9968ee..68f67b8a2743b 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats_count_distinct.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats_count_distinct.csv-spec @@ -117,7 +117,7 @@ m:long | languages:i 15 | 4 20 | 5 10 | null -; +; countDistinctOfIpGroupByKeyword from hosts | stats h0 = count_distinct(ip0), h1 = count_distinct(ip1) by host | sort host; @@ -128,3 +128,27 @@ h0:long | h1:long | host:keyword 5 | 6 | epsilon 1 | 2 | gamma ; + +countDistinctWithPrecisionExpression +from employees | stats m = count_distinct(height, 9875+1) by languages | sort languages; + +m:long | languages:i +13 | 1 +16 | 2 +14 | 3 +15 | 4 +20 | 5 +10 | null +; + +countDistinctWithComplexPrecisionExpression +from employees | stats m = count_distinct(height, 9876*3+(-9876*2)) by languages | sort languages; + +m:long | languages:i +13 | 1 +16 | 2 +14 | 3 +15 | 4 +20 | 5 +10 | null +; diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats_percentile.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats_percentile.csv-spec index eaa0786588480..6ab061b33dfb0 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats_percentile.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats_percentile.csv-spec @@ -142,3 +142,17 @@ MEDIAN(salary):double | MEDIAN_ABSOLUTE_DEVIATION(salary):double 47003 | 10096.5 // end::median-absolute-deviation-result[] ; + +medianViaExpression +from employees | stats p50 = percentile(salary_change, 25*2); + +p50:double +0.75 +; + +medianViaComplexExpression +from employees | stats p50 = percentile(salary_change, -(50-1)+99); + +p50:double +0.75 +; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java index 1f5e6ee3fd6ed..59c6e2782b014 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java @@ -125,24 +125,24 @@ else if (p.resolved()) { agg.aggregates().forEach(e -> { var exp = e instanceof Alias ? ((Alias) e).child() : e; if (exp instanceof AggregateFunction aggFunc) { - aggFunc.arguments().forEach(a -> { - // TODO: allow an expression? - if ((a instanceof FieldAttribute - || a instanceof MetadataAttribute - || a instanceof ReferenceAttribute - || a instanceof Literal) == false) { - failures.add( - fail( - e, - "aggregate function's parameters must be an attribute or literal; found [" - + a.sourceText() - + "] of type [" - + a.nodeName() - + "]" - ) - ); - } - }); + Expression field = aggFunc.field(); + + // TODO: allow an expression? + if ((field instanceof FieldAttribute + || field instanceof MetadataAttribute + || field instanceof ReferenceAttribute + || field instanceof Literal) == false) { + failures.add( + fail( + e, + "aggregate function's field must be an attribute or literal; found [" + + field.sourceText() + + "] of type [" + + field.nodeName() + + "]" + ) + ); + } } else if (agg.groupings().contains(exp) == false) { // TODO: allow an expression? failures.add( fail( diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountDistinct.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountDistinct.java index 044d89d41a0c5..2e20af1889773 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountDistinct.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountDistinct.java @@ -29,6 +29,7 @@ import static org.elasticsearch.xpack.ql.expression.TypeResolutions.ParamOrdinal.DEFAULT; import static org.elasticsearch.xpack.ql.expression.TypeResolutions.ParamOrdinal.SECOND; +import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isFoldable; import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isInteger; public class CountDistinct extends AggregateFunction implements OptionalArgument, ToAggregator { @@ -66,7 +67,7 @@ protected TypeResolution resolveType() { return resolution; } - return isInteger(precision, sourceText(), SECOND); + return isInteger(precision, sourceText(), SECOND).and(isFoldable(precision, sourceText(), SECOND)); } @Override diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Percentile.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Percentile.java index db560ff7043df..9e4eccb964de4 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Percentile.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Percentile.java @@ -56,11 +56,7 @@ protected TypeResolution resolveType() { return resolution; } - resolution = isNumeric(percentile, sourceText(), SECOND); - if (resolution.unresolved()) { - return resolution; - } - return isFoldable(percentile, sourceText(), SECOND); + return isNumeric(percentile, sourceText(), SECOND).and(isFoldable(percentile, sourceText(), SECOND)); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java index 3c1a9800d6d11..10f134432a0a2 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java @@ -68,7 +68,7 @@ public void testAggsExpressionsInStatsAggs() { error("from test | stats length(first_name), count(1) by first_name") ); assertEquals( - "1:19: aggregate function's parameters must be an attribute or literal; found [emp_no / 2] of type [Div]", + "1:19: aggregate function's field must be an attribute or literal; found [emp_no / 2] of type [Div]", error("from test | stats x = avg(emp_no / 2) by emp_no") ); assertEquals( @@ -76,13 +76,21 @@ public void testAggsExpressionsInStatsAggs() { error("from test | stats count(avg(first_name)) by first_name") ); assertEquals( - "1:19: aggregate function's parameters must be an attribute or literal; found [length(first_name)] of type [Length]", + "1:19: aggregate function's field must be an attribute or literal; found [length(first_name)] of type [Length]", error("from test | stats count(length(first_name)) by first_name") ); assertEquals( "1:23: expected an aggregate function or group but got [emp_no + avg(emp_no)] of type [Add]", error("from test | stats x = emp_no + avg(emp_no) by emp_no") ); + assertEquals( + "1:23: second argument of [percentile(languages, languages)] must be a constant, received [languages]", + error("from test | stats x = percentile(languages, languages) by emp_no") + ); + assertEquals( + "1:23: second argument of [count_distinct(languages, languages)] must be a constant, received [languages]", + error("from test | stats x = count_distinct(languages, languages) by emp_no") + ); } public void testDoubleRenamingField() { diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expression.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expression.java index 5f56694934f1b..9e95dab82df19 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expression.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expression.java @@ -50,6 +50,10 @@ public boolean resolved() { return failed == false; } + public TypeResolution and(TypeResolution other) { + return failed ? this : other; + } + public String message() { return message; }