Skip to content

Commit 9c52106

Browse files
authored
ESQL: Move explain command validation off EsqlSession (#129918)
1 parent ae3c360 commit 9c52106

File tree

6 files changed

+22
-26
lines changed

6 files changed

+22
-26
lines changed

muted-tests.yml

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -562,9 +562,6 @@ tests:
562562
- class: org.elasticsearch.xpack.rank.rrf.RRFRankClientYamlTestSuiteIT
563563
method: test {yaml=rrf/950_pinned_interaction/rrf with pinned retriever as a sub-retriever}
564564
issue: https://github.com/elastic/elasticsearch/issues/129845
565-
- class: org.elasticsearch.xpack.test.rest.XPackRestIT
566-
method: test {p0=esql/60_usage/Basic ESQL usage output (telemetry) non-snapshot version}
567-
issue: https://github.com/elastic/elasticsearch/issues/129888
568565
- class: org.elasticsearch.index.mapper.vectors.DenseVectorFieldMapperTests
569566
method: testExistsQueryMinimalMapping
570567
issue: https://github.com/elastic/elasticsearch/issues/129911

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,13 @@ public LogicalPlanBuilder(ParsingContext context) {
120120

121121
protected LogicalPlan plan(ParseTree ctx) {
122122
LogicalPlan p = ParserUtils.typedParsing(this, ctx, LogicalPlan.class);
123+
if (p instanceof Explain == false && p.anyMatch(logicalPlan -> logicalPlan instanceof Explain)) {
124+
throw new ParsingException(source(ctx), "EXPLAIN does not support downstream commands");
125+
}
126+
if (p instanceof Explain explain && explain.query().anyMatch(logicalPlan -> logicalPlan instanceof Explain)) {
127+
// TODO this one is never reached because the Parser fails to understand multiple round brackets
128+
throw new ParsingException(source(ctx), "EXPLAIN cannot be used inside another EXPLAIN command");
129+
}
123130
var errors = this.context.params().parsingErrors();
124131
if (errors.hasNext() == false) {
125132
return p;

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -190,15 +190,10 @@ public void execute(EsqlQueryRequest request, EsqlExecutionInfo executionInfo, P
190190
assert executionInfo != null : "Null EsqlExecutionInfo";
191191
LOGGER.debug("ESQL query:\n{}", request.query());
192192
LogicalPlan parsed = parse(request.query(), request.params());
193-
Explain explain = findExplain(parsed);
194-
if (explain != null) {
193+
if (parsed instanceof Explain explain) {
195194
explainMode = true;
196-
if (explain == parsed) {
197-
parsed = explain.query();
198-
parsedPlanString = parsed.toString();
199-
} else {
200-
throw new VerificationException("EXPLAIN does not support downstream commands");
201-
}
195+
parsed = explain.query();
196+
parsedPlanString = parsed.toString();
202197
}
203198
analyzedPlan(parsed, executionInfo, request.filter(), new EsqlCCSUtils.CssPartialErrorsActionListener(executionInfo, listener) {
204199
@Override
@@ -211,19 +206,6 @@ public void onResponse(LogicalPlan analyzedPlan) {
211206
});
212207
}
213208

214-
private Explain findExplain(LogicalPlan parsed) {
215-
if (parsed instanceof Explain e) {
216-
return e;
217-
}
218-
for (LogicalPlan child : parsed.children()) {
219-
Explain result = findExplain(child);
220-
if (result != null) {
221-
return result;
222-
}
223-
}
224-
return null;
225-
}
226-
227209
/**
228210
* Execute an analyzed plan. Most code should prefer calling {@link #execute} but
229211
* this is public for testing.

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3496,6 +3496,16 @@ public void testPreserveParentheses() {
34963496
}
34973497
}
34983498

3499+
public void testExplainErrors() {
3500+
assumeTrue("Requires EXPLAIN capability", EsqlCapabilities.Cap.EXPLAIN.isEnabled());
3501+
// TODO this one is incorrect
3502+
expectError("explain ( from test ) | limit 1", "line 1:23: mismatched input '|' expecting {'|', ',', ')', 'metadata'}");
3503+
expectError(
3504+
"explain (row x=\"Elastic\" | eval y=concat(x,to_upper(\"search\"))) | mv_expand y",
3505+
"line 1:1: EXPLAIN does not support downstream commands"
3506+
);
3507+
}
3508+
34993509
public void testRerankDefaultInferenceIdAndScoreAttribute() {
35003510
assumeTrue("RERANK requires corresponding capability", EsqlCapabilities.Cap.RERANK.isEnabled());
35013511

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/220_explain.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,5 +97,5 @@ explainDownstream:
9797
query: 'EXPLAIN (row a = 1) | eval b = 2'
9898
catch: "bad_request"
9999

100-
- match: { error.type: "verification_exception" }
100+
- match: { error.type: "parsing_exception" }
101101
- contains: { error.reason: "EXPLAIN does not support downstream commands" }

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ setup:
4646
- do: {xpack.usage: {}}
4747
- match: { esql.available: true }
4848
- match: { esql.enabled: true }
49-
- length: { esql.features: 26 }
49+
- length: { esql.features: 27 }
5050
- set: {esql.features.dissect: dissect_counter}
5151
- set: {esql.features.drop: drop_counter}
5252
- set: {esql.features.eval: eval_counter}

0 commit comments

Comments
 (0)