From ff10ec8c101556ff4615e49db197f9d86142b8f4 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Tue, 24 Jun 2025 15:44:46 +0300 Subject: [PATCH 1/4] Move explain command validation off EsqlSession --- .../xpack/esql/parser/LogicalPlanBuilder.java | 7 ++++++ .../xpack/esql/session/EsqlSession.java | 24 +++---------------- .../esql/parser/StatementParserTests.java | 7 ++++++ .../rest-api-spec/test/esql/220_explain.yml | 2 +- .../rest-api-spec/test/esql/60_usage.yml | 2 +- 5 files changed, 19 insertions(+), 23 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java index e56d006929551..a8ea1ba95dc15 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java @@ -120,6 +120,13 @@ public LogicalPlanBuilder(ParsingContext context) { protected LogicalPlan plan(ParseTree ctx) { LogicalPlan p = ParserUtils.typedParsing(this, ctx, LogicalPlan.class); + if (p instanceof Explain == false && p.anyMatch(logicalPlan -> logicalPlan instanceof Explain)) { + throw new ParsingException(source(ctx), "EXPLAIN does not support downstream commands"); + } + if (p instanceof Explain explain && explain.query().anyMatch(logicalPlan -> logicalPlan instanceof Explain)) { + // TODO this one is never reached because the Parser fails to understand multiple round brackets + throw new ParsingException(source(ctx), "EXPLAIN cannot be used inside another EXPLAIN command"); + } var errors = this.context.params().parsingErrors(); if (errors.hasNext() == false) { return p; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java index 6e246785f0b6a..4ff65f59bbd72 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java @@ -190,15 +190,10 @@ public void execute(EsqlQueryRequest request, EsqlExecutionInfo executionInfo, P assert executionInfo != null : "Null EsqlExecutionInfo"; LOGGER.debug("ESQL query:\n{}", request.query()); LogicalPlan parsed = parse(request.query(), request.params()); - Explain explain = findExplain(parsed); - if (explain != null) { + if (parsed instanceof Explain explain) { explainMode = true; - if (explain == parsed) { - parsed = explain.query(); - parsedPlanString = parsed.toString(); - } else { - throw new VerificationException("EXPLAIN does not support downstream commands"); - } + parsed = explain.query(); + parsedPlanString = parsed.toString(); } analyzedPlan(parsed, executionInfo, request.filter(), new EsqlCCSUtils.CssPartialErrorsActionListener(executionInfo, listener) { @Override @@ -211,19 +206,6 @@ public void onResponse(LogicalPlan analyzedPlan) { }); } - private Explain findExplain(LogicalPlan parsed) { - if (parsed instanceof Explain e) { - return e; - } - for (LogicalPlan child : parsed.children()) { - Explain result = findExplain(child); - if (result != null) { - return result; - } - } - return null; - } - /** * Execute an analyzed plan. Most code should prefer calling {@link #execute} but * this is public for testing. 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 b0f83672b5efb..7aef0f57efffd 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 @@ -3496,6 +3496,13 @@ public void testPreserveParentheses() { } } + public void testExplainErrors() { + assumeTrue("Requires EXPLAIN capability", EsqlCapabilities.Cap.EXPLAIN.isEnabled()); + // TODO this one is incorrect + expectError("explain ( from test ) | limit 1", "line 1:23: mismatched input '|' expecting {'|', ',', ')', 'metadata'}"); + expectError("explain (row x=\"Elastic\" | eval y=concat(x,to_upper(\"search\"))) | mv_expand y", "line 1:1: EXPLAIN does not support downstream commands"); + } + public void testRerankDefaultInferenceIdAndScoreAttribute() { assumeTrue("RERANK requires corresponding capability", EsqlCapabilities.Cap.RERANK.isEnabled()); diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/220_explain.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/220_explain.yml index e6b68c7f682a9..be9163bd2a2fa 100644 --- a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/220_explain.yml +++ b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/220_explain.yml @@ -97,5 +97,5 @@ explainDownstream: query: 'EXPLAIN (row a = 1) | eval b = 2' catch: "bad_request" - - match: { error.type: "verification_exception" } + - match: { error.type: "parsing_exception" } - contains: { error.reason: "EXPLAIN does not support downstream commands" } diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml index 3aca6d2ff500b..1276f026ffdf7 100644 --- a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml +++ b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml @@ -46,7 +46,7 @@ setup: - do: {xpack.usage: {}} - match: { esql.available: true } - match: { esql.enabled: true } - - length: { esql.features: 26 } + - length: { esql.features: 27 } - set: {esql.features.dissect: dissect_counter} - set: {esql.features.drop: drop_counter} - set: {esql.features.eval: eval_counter} From 253f35c4d747d892b2a7ac464a4644c74f111a6a Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Tue, 24 Jun 2025 12:58:12 +0000 Subject: [PATCH 2/4] [CI] Auto commit changes from spotless --- .../xpack/esql/parser/StatementParserTests.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 7aef0f57efffd..cb5c531939498 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 @@ -3500,7 +3500,10 @@ public void testExplainErrors() { assumeTrue("Requires EXPLAIN capability", EsqlCapabilities.Cap.EXPLAIN.isEnabled()); // TODO this one is incorrect expectError("explain ( from test ) | limit 1", "line 1:23: mismatched input '|' expecting {'|', ',', ')', 'metadata'}"); - expectError("explain (row x=\"Elastic\" | eval y=concat(x,to_upper(\"search\"))) | mv_expand y", "line 1:1: EXPLAIN does not support downstream commands"); + expectError( + "explain (row x=\"Elastic\" | eval y=concat(x,to_upper(\"search\"))) | mv_expand y", + "line 1:1: EXPLAIN does not support downstream commands" + ); } public void testRerankDefaultInferenceIdAndScoreAttribute() { From 84ec0a3f23c21d4ed2606891ad82dccfb27a0313 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Tue, 24 Jun 2025 16:02:09 +0300 Subject: [PATCH 3/4] Revert fix for usage --- .../yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml index 1276f026ffdf7..3aca6d2ff500b 100644 --- a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml +++ b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml @@ -46,7 +46,7 @@ setup: - do: {xpack.usage: {}} - match: { esql.available: true } - match: { esql.enabled: true } - - length: { esql.features: 27 } + - length: { esql.features: 26 } - set: {esql.features.dissect: dissect_counter} - set: {esql.features.drop: drop_counter} - set: {esql.features.eval: eval_counter} From 7b67ce0298316c5594f31651f3840c80f60db244 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Tue, 24 Jun 2025 16:29:38 +0300 Subject: [PATCH 4/4] put back the fix for telemetry usage tests --- muted-tests.yml | 3 --- .../resources/rest-api-spec/test/esql/60_usage.yml | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index 67ff655e0495e..46e00e16b1d71 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -562,9 +562,6 @@ tests: - class: org.elasticsearch.xpack.rank.rrf.RRFRankClientYamlTestSuiteIT method: test {yaml=rrf/950_pinned_interaction/rrf with pinned retriever as a sub-retriever} issue: https://github.com/elastic/elasticsearch/issues/129845 -- class: org.elasticsearch.xpack.test.rest.XPackRestIT - method: test {p0=esql/60_usage/Basic ESQL usage output (telemetry) non-snapshot version} - issue: https://github.com/elastic/elasticsearch/issues/129888 - class: org.elasticsearch.index.mapper.vectors.DenseVectorFieldMapperTests method: testExistsQueryMinimalMapping issue: https://github.com/elastic/elasticsearch/issues/129911 diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml index 3aca6d2ff500b..1276f026ffdf7 100644 --- a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml +++ b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml @@ -46,7 +46,7 @@ setup: - do: {xpack.usage: {}} - match: { esql.available: true } - match: { esql.enabled: true } - - length: { esql.features: 26 } + - length: { esql.features: 27 } - set: {esql.features.dissect: dissect_counter} - set: {esql.features.drop: drop_counter} - set: {esql.features.eval: eval_counter}