From 33137907cf7dd6cdc81bbc22a977f55ac0436b85 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 3 Jan 2019 13:55:09 +0200 Subject: [PATCH] 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 --- docs/reference/sql/functions/aggs.asciidoc | 4 +-- .../function/aggregate/Percentile.java | 15 +++++++---- .../function/aggregate/PercentileRank.java | 6 +++++ .../analyzer/VerifierErrorMessagesTests.java | 26 ++++++++++++++++--- 4 files changed, 40 insertions(+), 11 deletions(-) diff --git a/docs/reference/sql/functions/aggs.asciidoc b/docs/reference/sql/functions/aggs.asciidoc index d55da5a45a0..aecb557d8eb 100644 --- a/docs/reference/sql/functions/aggs.asciidoc +++ b/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 diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Percentile.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Percentile.java index 6e644fb4f75..47deaa501cf 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Percentile.java +++ b/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 class Percentile extends NumericAggregate implements EnclosedAgg { @Override protected TypeResolution resolveType() { - TypeResolution resolution = super.resolveType(); - - if (TypeResolution.TYPE_RESOLVED.equals(resolution)) { - resolution = Expressions.typeMustBeNumeric(percent(), functionName(), ParamOrdinal.DEFAULT); + if (!percent.foldable()) { + throw new SqlIllegalArgumentException("2nd argument of PERCENTILE must be constant, received [{}]", + Expressions.name(percent)); } - return resolution; + TypeResolution resolution = super.resolveType(); + if (resolution.unresolved()) { + return resolution; + } + + return Expressions.typeMustBeNumeric(percent, functionName(), ParamOrdinal.DEFAULT); } public Expression percent() { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/PercentileRank.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/PercentileRank.java index f01dad8800c..7fa5e2e4355 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/PercentileRank.java +++ b/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 class PercentileRank extends AggregateFunction implements EnclosedAgg { @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; diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index 5a786441d33..b5403d21d5a 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/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 org.elasticsearch.xpack.sql.type.TypesTests; 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 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 class VerifierErrorMessagesTests extends ESTestCase { assertEquals("1:47: Cannot use an aggregate [MAX] for grouping", error("SELECT MAX(date) FROM test GROUP BY HISTOGRAM(MAX(int), 1)")); } -} \ No newline at end of file + + 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()); + } +}