From 40db731cb671744d5a905b166032aa3bc663388a Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Thu, 12 Sep 2024 18:20:01 +0300 Subject: [PATCH] Fix error messages --- .../xpack/esql/analysis/AnalyzerTests.java | 16 +++--- .../xpack/esql/analysis/VerifierTests.java | 54 +++++++++++++------ .../scalar/conditional/CaseTests.java | 44 ++++++++------- .../optimizer/LogicalPlanOptimizerTests.java | 19 ++++--- .../optimizer/PhysicalPlanOptimizerTests.java | 12 ++--- .../esql/parser/StatementParserTests.java | 38 ++++++++----- .../session/IndexResolverFieldNamesTests.java | 12 ++++- 7 files changed, 125 insertions(+), 70 deletions(-) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java index b534320f60c10..a14c6bf22d532 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java @@ -1904,9 +1904,9 @@ public void testLookup() { | RENAME languages AS int | LOOKUP int_number_names ON int """; - if (Build.current().isProductionRelease()) { + if (Build.current().isSnapshot() == false) { var e = expectThrows(ParsingException.class, () -> analyze(query)); - assertThat(e.getMessage(), containsString("line 3:4: LOOKUP is in preview and only available in SNAPSHOT build")); + assertThat(e.getMessage(), containsString("line 3:3: mismatched input 'LOOKUP' expecting {")); return; } LogicalPlan plan = analyze(query); @@ -1960,9 +1960,9 @@ public void testLookupMissingField() { FROM test | LOOKUP int_number_names ON garbage """; - if (Build.current().isProductionRelease()) { + if (Build.current().isSnapshot() == false) { var e = expectThrows(ParsingException.class, () -> analyze(query)); - assertThat(e.getMessage(), containsString("line 2:4: LOOKUP is in preview and only available in SNAPSHOT build")); + assertThat(e.getMessage(), containsString("line 2:3: mismatched input 'LOOKUP' expecting {")); return; } var e = expectThrows(VerificationException.class, () -> analyze(query)); @@ -1974,9 +1974,9 @@ public void testLookupMissingTable() { FROM test | LOOKUP garbage ON a """; - if (Build.current().isProductionRelease()) { + if (Build.current().isSnapshot() == false) { var e = expectThrows(ParsingException.class, () -> analyze(query)); - assertThat(e.getMessage(), containsString("line 2:4: LOOKUP is in preview and only available in SNAPSHOT build")); + assertThat(e.getMessage(), containsString("line 2:3: mismatched input 'LOOKUP' expecting {")); return; } var e = expectThrows(VerificationException.class, () -> analyze(query)); @@ -1989,9 +1989,9 @@ public void testLookupMatchTypeWrong() { | RENAME last_name AS int | LOOKUP int_number_names ON int """; - if (Build.current().isProductionRelease()) { + if (Build.current().isSnapshot() == false) { var e = expectThrows(ParsingException.class, () -> analyze(query)); - assertThat(e.getMessage(), containsString("line 3:4: LOOKUP is in preview and only available in SNAPSHOT build")); + assertThat(e.getMessage(), containsString("line 3:3: mismatched input 'LOOKUP' expecting {")); return; } var e = expectThrows(VerificationException.class, () -> analyze(query)); 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 4cf6ae2c3986b..f789eab605c3b 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 @@ -18,6 +18,7 @@ import org.elasticsearch.xpack.esql.index.EsIndex; import org.elasticsearch.xpack.esql.index.IndexResolution; import org.elasticsearch.xpack.esql.parser.EsqlParser; +import org.elasticsearch.xpack.esql.parser.ParsingException; import org.elasticsearch.xpack.esql.parser.QueryParam; import org.elasticsearch.xpack.esql.parser.QueryParams; @@ -35,6 +36,7 @@ import static org.elasticsearch.xpack.esql.core.type.DataType.UNSIGNED_LONG; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.matchesRegex; //@TestLogging(value = "org.elasticsearch.xpack.esql:TRACE,org.elasticsearch.compute:TRACE", reason = "debug") @@ -1075,21 +1077,36 @@ public void testMatchFilter() throws Exception { ); } - public void testMatchCommand() throws Exception { - assumeTrue("skipping because MATCH_COMMAND is not enabled", EsqlCapabilities.Cap.MATCH_COMMAND.isEnabled()); - assertEquals("1:24: MATCH cannot be used after LIMIT", error("from test | limit 10 | match \"Anna\"")); - assertEquals("1:13: MATCH cannot be used after SHOW", error("show info | match \"8.16.0\"")); - assertEquals("1:17: MATCH cannot be used after ROW", error("row a= \"Anna\" | match \"Anna\"")); - assertEquals("1:26: MATCH cannot be used after EVAL", error("from test | eval z = 2 | match \"Anna\"")); - assertEquals("1:43: MATCH cannot be used after DISSECT", error("from test | dissect first_name \"%{foo}\" | match \"Connection\"")); - assertEquals("1:27: MATCH cannot be used after DROP", error("from test | drop emp_no | match \"Anna\"")); - assertEquals("1:35: MATCH cannot be used after EVAL", error("from test | eval n = emp_no * 3 | match \"Anna\"")); - assertEquals("1:44: MATCH cannot be used after GROK", error("from test | grok last_name \"%{WORD:foo}\" | match \"Anna\"")); - assertEquals("1:27: MATCH cannot be used after KEEP", error("from test | keep emp_no | match \"Anna\"")); + public void testMatchCommand() { + assertMatchCommand("1:24:", "LIMIT","from test | limit 10 | match \"Anna\""); + assertMatchCommand("1:13:", "SHOW", "show info | match \"8.16.0\""); + assertMatchCommand("1:17:", "ROW","row a= \"Anna\" | match \"Anna\""); + assertMatchCommand("1:26:", "EVAL","from test | eval z = 2 | match \"Anna\""); + assertMatchCommand("1:43:","DISSECT","from test | dissect first_name \"%{foo}\" | match \"Connection\""); + assertMatchCommand("1:27:", "DROP","from test | drop emp_no | match \"Anna\""); + assertMatchCommand("1:35:", "EVAL", "from test | eval n = emp_no * 3 | match \"Anna\""); + assertMatchCommand("1:44:", "GROK","from test | grok last_name \"%{WORD:foo}\" | match \"Anna\""); + assertMatchCommand("1:27:", "KEEP","from test | keep emp_no | match \"Anna\""); // TODO Keep adding tests for all unsupported commands } + private void assertMatchCommand(String lineAndColumn, String command, String query) { + String message; + Class exception; + var isSnapshot = Build.current().isSnapshot(); + if (isSnapshot) { + message = " MATCH cannot be used after "; + exception = VerificationException.class; + } else { + message = " mismatched input 'match' expecting "; + exception = ParsingException.class; + } + + var expectedErrorMessage = lineAndColumn + message + (isSnapshot ? command : ""); + assertThat(error(query, defaultAnalyzer, exception), containsString(expectedErrorMessage)); + } + public void testCoalesceWithMixedNumericTypes() { assertEquals( "1:22: second argument of [coalesce(languages, height)] must be [integer], found value [height] type [double]", @@ -1292,6 +1309,10 @@ private String error(String query, Object... params) { } private String error(String query, Analyzer analyzer, Object... params) { + return error(query, analyzer, VerificationException.class, params); + } + + private String error(String query, Analyzer analyzer, Class exception, Object... params) { List parameters = new ArrayList<>(); for (Object param : params) { if (param == null) { @@ -1304,12 +1325,13 @@ private String error(String query, Analyzer analyzer, Object... params) { throw new IllegalArgumentException("VerifierTests don't support params of type " + param.getClass()); } } - VerificationException e = expectThrows( - VerificationException.class, - () -> analyzer.analyze(parser.createStatement(query, new QueryParams(parameters))) - ); + Throwable e = expectThrows(exception, () -> analyzer.analyze(parser.createStatement(query, new QueryParams(parameters)))); + assertThat(e, instanceOf(exception)); + String message = e.getMessage(); - assertTrue(message.startsWith("Found ")); + if (e instanceof VerificationException) { + assertTrue(message.startsWith("Found ")); + } String pattern = "\nline "; int index = message.indexOf(pattern); return message.substring(index + pattern.length()); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java index 7b26ac8c099dc..b4c224536a98f 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java @@ -10,6 +10,7 @@ import com.carrotsearch.randomizedtesting.annotations.Name; import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; +import org.elasticsearch.Build; import org.elasticsearch.core.Nullable; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.esql.core.expression.Expression; @@ -34,24 +35,31 @@ import static org.hamcrest.Matchers.startsWith; public class CaseTests extends AbstractScalarFunctionTestCase { - private static final List TYPES = List.of( - DataType.KEYWORD, - DataType.TEXT, - DataType.BOOLEAN, - DataType.DATETIME, - DataType.DATE_NANOS, - DataType.DOUBLE, - DataType.INTEGER, - DataType.LONG, - DataType.UNSIGNED_LONG, - DataType.IP, - DataType.VERSION, - DataType.CARTESIAN_POINT, - DataType.GEO_POINT, - DataType.CARTESIAN_SHAPE, - DataType.GEO_SHAPE, - DataType.NULL - ); + + private static final List TYPES; + static { + List t = List.of( + DataType.KEYWORD, + DataType.TEXT, + DataType.BOOLEAN, + DataType.DATETIME, + DataType.DOUBLE, + DataType.INTEGER, + DataType.LONG, + DataType.UNSIGNED_LONG, + DataType.IP, + DataType.VERSION, + DataType.CARTESIAN_POINT, + DataType.GEO_POINT, + DataType.CARTESIAN_SHAPE, + DataType.GEO_SHAPE, + DataType.NULL + ); + if (Build.current().isSnapshot()) { + t.add(DataType.DATE_NANOS); + } + TYPES = new ArrayList<>(t); + } public CaseTests(@Name("TestCase") Supplier testCaseSupplier) { this.testCase = testCaseSupplier.get(); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index 22a4b410a6d7a..6ff541de1854a 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -4431,11 +4431,16 @@ public void testReplaceSortByExpressionsWithStats() { * \_EsRelation[test][_meta_field{f}#13, emp_no{f}#7, first_name{f}#8, ge..] */ public void testInlinestatsNestedExpressionsInGroups() { - var plan = optimizedPlan(""" + var query = """ FROM test | INLINESTATS c = COUNT(salary) by emp_no % 2 - """); - + """; + if (Build.current().isSnapshot() == false) { + var e = expectThrows(ParsingException.class, () -> analyze(query)); + assertThat(e.getMessage(), containsString("line 2:3: mismatched input 'INLINESTATS' expecting {")); + return; + } + var plan = optimizedPlan(query); var limit = as(plan, Limit.class); var agg = as(limit.child(), InlineStats.class); var groupings = agg.groupings(); @@ -4862,9 +4867,9 @@ public void testLookupSimple() { FROM test | RENAME languages AS int | LOOKUP int_number_names ON int"""; - if (Build.current().isProductionRelease()) { + if (Build.current().isSnapshot() == false) { var e = expectThrows(ParsingException.class, () -> analyze(query)); - assertThat(e.getMessage(), containsString("line 3:4: LOOKUP is in preview and only available in SNAPSHOT build")); + assertThat(e.getMessage(), containsString("line 3:3: mismatched input 'LOOKUP' expecting {")); return; } var plan = optimizedPlan(query); @@ -4941,9 +4946,9 @@ public void testLookupStats() { | RENAME languages AS int | LOOKUP int_number_names ON int | STATS MIN(emp_no) BY name"""; - if (Build.current().isProductionRelease()) { + if (Build.current().isSnapshot() == false) { var e = expectThrows(ParsingException.class, () -> analyze(query)); - assertThat(e.getMessage(), containsString("line 3:4: LOOKUP is in preview and only available in SNAPSHOT build")); + assertThat(e.getMessage(), containsString("line 3:3: mismatched input 'LOOKUP' expecting {")); return; } var plan = optimizedPlan(query); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java index 96e8cd68ae9d2..3a99402c9e657 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java @@ -4527,9 +4527,9 @@ public void testLookupSimple() { FROM test | RENAME languages AS int | LOOKUP int_number_names ON int"""; - if (Build.current().isProductionRelease()) { + if (Build.current().isSnapshot() == false) { var e = expectThrows(ParsingException.class, () -> analyze(query)); - assertThat(e.getMessage(), containsString("line 3:4: LOOKUP is in preview and only available in SNAPSHOT build")); + assertThat(e.getMessage(), containsString("line 3:3: mismatched input 'LOOKUP' expecting {")); return; } PhysicalPlan plan = physicalPlan(query); @@ -4576,9 +4576,9 @@ public void testLookupThenProject() { | LOOKUP int_number_names ON int | RENAME int AS languages, name AS lang_name | KEEP emp_no, languages, lang_name"""; - if (Build.current().isProductionRelease()) { + if (Build.current().isSnapshot() == false) { var e = expectThrows(ParsingException.class, () -> analyze(query)); - assertThat(e.getMessage(), containsString("line 5:4: LOOKUP is in preview and only available in SNAPSHOT build")); + assertThat(e.getMessage(), containsString("line 5:3: mismatched input 'LOOKUP' expecting {")); return; } PhysicalPlan plan = optimizedPlan(physicalPlan(query)); @@ -4633,9 +4633,9 @@ public void testLookupThenTopN() { | RENAME name AS languages | KEEP languages, emp_no | SORT languages ASC, emp_no ASC"""; - if (Build.current().isProductionRelease()) { + if (Build.current().isSnapshot() == false) { var e = expectThrows(ParsingException.class, () -> analyze(query)); - assertThat(e.getMessage(), containsString("line 3:4: LOOKUP is in preview and only available in SNAPSHOT build")); + assertThat(e.getMessage(), containsString("line 3:3: mismatched input 'LOOKUP' expecting {")); return; } var plan = physicalPlan(query); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java index 9af152116e1e1..3ee7509ea1530 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java @@ -76,6 +76,8 @@ //@TestLogging(value = "org.elasticsearch.xpack.esql:TRACE", reason = "debug") public class StatementParserTests extends AbstractStatementParserTests { + private static final LogicalPlan PROCESSING_CMD_INPUT = new Row(EMPTY, List.of(new Alias(EMPTY, "a", integer(1)))); + public void testRowCommand() { assertEquals( new Row(EMPTY, List.of(new Alias(EMPTY, "a", integer(1)), new Alias(EMPTY, "b", integer(2)))), @@ -313,7 +315,12 @@ public void testAggsWithGroupKeyAsAgg() throws Exception { } public void testInlineStatsWithGroups() { - assumeTrue("INLINESTATS requires snapshot builds", Build.current().isSnapshot()); + var query = "inlinestats b = min(a) by c, d.e"; + if (Build.current().isSnapshot() == false) { + var e = expectThrows(ParsingException.class, () -> processingCommand(query)); + assertThat(e.getMessage(), containsString("line 1:13: mismatched input 'inlinestats' expecting {")); + return; + } assertEquals( new InlineStats( EMPTY, @@ -325,12 +332,17 @@ public void testInlineStatsWithGroups() { attribute("d.e") ) ), - processingCommand("inlinestats b = min(a) by c, d.e") + processingCommand(query) ); } public void testInlineStatsWithoutGroups() { - assumeTrue("INLINESTATS requires snapshot builds", Build.current().isSnapshot()); + var query = "inlinestats min(a), c = 1"; + if (Build.current().isSnapshot() == false) { + var e = expectThrows(ParsingException.class, () -> processingCommand(query)); + assertThat(e.getMessage(), containsString("line 1:13: mismatched input 'inlinestats' expecting {")); + return; + } assertEquals( new InlineStats( EMPTY, @@ -341,7 +353,7 @@ public void testInlineStatsWithoutGroups() { new Alias(EMPTY, "c", integer(1)) ) ), - processingCommand("inlinestats min(a), c = 1") + processingCommand(query) ); } @@ -389,6 +401,7 @@ public void testStringAsIndexPattern() { } public void testStringAsLookupIndexPattern() { + assumeTrue("requires snapshot build", Build.current().isSnapshot()); assertStringAsLookupIndexPattern("foo", "ROW x = 1 | LOOKUP \"foo\" ON j"); assertStringAsLookupIndexPattern("test-*", """ ROW x = 1 | LOOKUP "test-*" ON j @@ -440,6 +453,7 @@ public void testInvalidQuotingAsFromIndexPattern() { } public void testInvalidQuotingAsMetricsIndexPattern() { + assumeTrue("requires snapshot build", Build.current().isSnapshot()); expectError("METRICS \"foo", ": token recognition error at: '\"foo'"); expectError("METRICS \"foo | LIMIT 1", ": token recognition error at: '\"foo | LIMIT 1'"); expectError("METRICS \"\"\"foo", ": token recognition error at: '\"'"); @@ -456,6 +470,7 @@ public void testInvalidQuotingAsMetricsIndexPattern() { } public void testInvalidQuotingAsLookupIndexPattern() { + assumeTrue("requires snapshot builds", Build.current().isSnapshot()); expectError("ROW x = 1 | LOOKUP \"foo ON j", ": token recognition error at: '\"foo ON j'"); expectError("ROW x = 1 | LOOKUP \"\"\"foo ON j", ": token recognition error at: '\"foo ON j'"); @@ -1453,9 +1468,9 @@ public void testQuotedName() { } private void assertStringAsIndexPattern(String string, String statement) { - if (Build.current().isProductionRelease() && statement.contains("METRIC")) { - var e = expectThrows(IllegalArgumentException.class, () -> statement(statement)); - assertThat(e.getMessage(), containsString("METRICS command currently requires a snapshot build")); + if (Build.current().isSnapshot() == false && statement.contains("METRIC")) { + var e = expectThrows(ParsingException.class, () -> statement(statement)); + assertThat(e.getMessage(), containsString("mismatched input 'METRICS' expecting {")); return; } LogicalPlan from = statement(statement); @@ -1465,7 +1480,7 @@ private void assertStringAsIndexPattern(String string, String statement) { } private void assertStringAsLookupIndexPattern(String string, String statement) { - if (Build.current().isProductionRelease()) { + if (Build.current().isSnapshot() == false) { var e = expectThrows(ParsingException.class, () -> statement(statement)); assertThat(e.getMessage(), containsString("line 1:14: LOOKUP is in preview and only available in SNAPSHOT build")); return; @@ -1533,9 +1548,9 @@ public void testInlineConvertWithNonexistentType() { public void testLookup() { String query = "ROW a = 1 | LOOKUP t ON j"; - if (Build.current().isProductionRelease()) { + if (Build.current().isSnapshot() == false) { var e = expectThrows(ParsingException.class, () -> statement(query)); - assertThat(e.getMessage(), containsString("line 1:14: LOOKUP is in preview and only available in SNAPSHOT build")); + assertThat(e.getMessage(), containsString("line 1:13: mismatched input 'LOOKUP' expecting {")); return; } var plan = statement(query); @@ -1715,7 +1730,4 @@ public void testMetricWithGroupKeyAsAgg() { expectVerificationError(query, "grouping key [a] already specified in the STATS BY clause"); } } - - private static final LogicalPlan PROCESSING_CMD_INPUT = new Row(EMPTY, List.of(new Alias(EMPTY, "a", integer(1)))); - } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/session/IndexResolverFieldNamesTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/session/IndexResolverFieldNamesTests.java index d0d800431360f..5425f770c49e8 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/session/IndexResolverFieldNamesTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/session/IndexResolverFieldNamesTests.java @@ -7,14 +7,17 @@ package org.elasticsearch.xpack.esql.session; +import org.elasticsearch.Build; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.esql.parser.EsqlParser; +import org.elasticsearch.xpack.esql.parser.ParsingException; import java.util.Collections; import java.util.Set; import static org.elasticsearch.xpack.esql.session.IndexResolver.ALL_FIELDS; import static org.elasticsearch.xpack.esql.session.IndexResolver.INDEX_METADATA_FIELD; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; public class IndexResolverFieldNamesTests extends ESTestCase { @@ -1228,8 +1231,13 @@ public void testEnrichOnDefaultField() { } public void testMetrics() { - Set fieldNames = EsqlSession.fieldNames(parser.createStatement(""" - METRICS k8s bytes=sum(rate(network.total_bytes_in)), sum(rate(network.total_cost)) BY cluster"""), Set.of()); + var query = "METRICS k8s bytes=sum(rate(network.total_bytes_in)), sum(rate(network.total_cost)) BY cluster"; + if (Build.current().isSnapshot() == false) { + var e = expectThrows(ParsingException.class, () -> parser.createStatement(query)); + assertThat(e.getMessage(), containsString("line 1:1: mismatched input 'METRICS' expecting {")); + return; + } + Set fieldNames = EsqlSession.fieldNames(parser.createStatement(query), Set.of()); assertThat( fieldNames, equalTo(