From 8a6acb88ccace9e2193c80c7d103642518b37f23 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 24 Jan 2019 10:34:17 +0200 Subject: [PATCH 1/2] SQL: Improve handling of invalide args for PERCENTILE/PERCENTILE_RANK Improve the Exception and the error message returned when 2nd argument of PERCENTILE and PERCENTILE_RANK is not a constant. --- .../expression/function/aggregate/Percentile.java | 6 +++--- .../function/aggregate/PercentileRank.java | 6 +++--- .../analyzer/VerifierErrorMessagesTests.java | 15 ++++----------- 3 files changed, 10 insertions(+), 17 deletions(-) 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 593466f4c4773..a8b4703840d90 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,7 +5,6 @@ */ 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; @@ -17,6 +16,7 @@ import java.util.List; import static java.util.Collections.singletonList; +import static org.elasticsearch.common.logging.LoggerMessageFormat.format; public class Percentile extends NumericAggregate implements EnclosedAgg { @@ -43,8 +43,8 @@ public Percentile replaceChildren(List newChildren) { @Override protected TypeResolution resolveType() { if (!percent.foldable()) { - throw new SqlIllegalArgumentException("2nd argument of PERCENTILE must be constant, received [{}]", - Expressions.name(percent)); + return new TypeResolution(format(null, "2nd argument of PERCENTILE must be constant, received [{}]", + Expressions.name(percent))); } TypeResolution resolution = super.resolveType(); 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 72614f8265f48..b3f7ab97e3ffc 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,7 +5,6 @@ */ 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; @@ -17,6 +16,7 @@ import java.util.List; import static java.util.Collections.singletonList; +import static org.elasticsearch.common.logging.LoggerMessageFormat.format; public class PercentileRank extends AggregateFunction implements EnclosedAgg { @@ -43,8 +43,8 @@ public Expression replaceChildren(List newChildren) { @Override protected TypeResolution resolveType() { if (!value.foldable()) { - throw new SqlIllegalArgumentException("2nd argument of PERCENTILE_RANK must be constant, received [{}]", - Expressions.name(value)); + return new TypeResolution(format(null, "2nd argument of PERCENTILE_RANK must be constant, received [{}]", + Expressions.name(value))); } TypeResolution resolution = super.resolveType(); 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 e45da9d08fee9..0de76d700d36b 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,7 +6,6 @@ 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; @@ -533,19 +532,13 @@ public void testAggsInHistogram() { } 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()); + assertEquals("1:8: 2nd argument of PERCENTILE must be constant, received [ABS(int)]", + error("SELECT PERCENTILE(int, ABS(int)) FROM test")); } 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()); + assertEquals("1:8: 2nd argument of PERCENTILE_RANK must be constant, received [ABS(int)]", + error("SELECT PERCENTILE_RANK(int, ABS(int)) FROM test")); } } From 078dd77392c445aaaeef471b6fef6a31baae7d60 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 24 Jan 2019 12:04:30 +0200 Subject: [PATCH 2/2] Address comment --- .../xpack/sql/expression/function/aggregate/Percentile.java | 2 +- .../sql/expression/function/aggregate/PercentileRank.java | 2 +- .../sql/analysis/analyzer/VerifierErrorMessagesTests.java | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) 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 a8b4703840d90..295932cd99c5e 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 @@ -43,7 +43,7 @@ public Percentile replaceChildren(List newChildren) { @Override protected TypeResolution resolveType() { if (!percent.foldable()) { - return new TypeResolution(format(null, "2nd argument of PERCENTILE must be constant, received [{}]", + return new TypeResolution(format(null, "2nd argument of PERCENTILE must be a constant, received [{}]", Expressions.name(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 b3f7ab97e3ffc..92bc794b248da 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 @@ -43,7 +43,7 @@ public Expression replaceChildren(List newChildren) { @Override protected TypeResolution resolveType() { if (!value.foldable()) { - return new TypeResolution(format(null, "2nd argument of PERCENTILE_RANK must be constant, received [{}]", + return new TypeResolution(format(null, "2nd argument of PERCENTILE_RANK must be a constant, received [{}]", Expressions.name(value))); } 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 0de76d700d36b..f66f8b0ea6147 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 @@ -532,12 +532,12 @@ public void testAggsInHistogram() { } public void testErrorMessageForPercentileWithSecondArgBasedOnAField() { - assertEquals("1:8: 2nd argument of PERCENTILE must be constant, received [ABS(int)]", + assertEquals("1:8: 2nd argument of PERCENTILE must be a constant, received [ABS(int)]", error("SELECT PERCENTILE(int, ABS(int)) FROM test")); } public void testErrorMessageForPercentileRankWithSecondArgBasedOnAField() { - assertEquals("1:8: 2nd argument of PERCENTILE_RANK must be constant, received [ABS(int)]", + assertEquals("1:8: 2nd argument of PERCENTILE_RANK must be a constant, received [ABS(int)]", error("SELECT PERCENTILE_RANK(int, ABS(int)) FROM test")); } }