Skip to content

Commit 6dd3dd5

Browse files
qianheng-awsqianheng-aws
authored
Separate explain mode from format params (#5042) (#5046)
* Separate explain mode from format params and support different combination * Fix CI * Fix CI * Fix CI * Support explain format BWC * Address comments and increase test coverage --------- (cherry picked from commit 5dbcd28) Signed-off-by: Heng Qian <qianheng@amazon.com>
1 parent 57e7ea7 commit 6dd3dd5

41 files changed

Lines changed: 523 additions & 152 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎core/src/main/java/org/opensearch/sql/ast/statement/Explain.java‎

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
package org.opensearch.sql.ast.statement;
1010

11-
import java.util.Locale;
1211
import lombok.EqualsAndHashCode;
1312
import lombok.Getter;
1413
import org.opensearch.sql.ast.AbstractNodeVisitor;
@@ -21,37 +20,20 @@ public class Explain extends Statement {
2120

2221
private final Statement statement;
2322
private final QueryType queryType;
24-
private final ExplainFormat format;
23+
private final ExplainMode mode;
2524

2625
public Explain(Statement statement, QueryType queryType) {
2726
this(statement, queryType, null);
2827
}
2928

30-
public Explain(Statement statement, QueryType queryType, String format) {
29+
public Explain(Statement statement, QueryType queryType, String mode) {
3130
this.statement = statement;
3231
this.queryType = queryType;
33-
this.format = Explain.format(format);
32+
this.mode = ExplainMode.of(mode);
3433
}
3534

3635
@Override
3736
public <R, C> R accept(AbstractNodeVisitor<R, C> visitor, C context) {
3837
return visitor.visitExplain(this, context);
3938
}
40-
41-
public enum ExplainFormat {
42-
SIMPLE,
43-
STANDARD,
44-
EXTENDED,
45-
COST,
46-
/** Formats explain output in yaml format. */
47-
YAML
48-
}
49-
50-
public static ExplainFormat format(String format) {
51-
try {
52-
return ExplainFormat.valueOf(format.toUpperCase(Locale.ROOT));
53-
} catch (Exception e) {
54-
return ExplainFormat.STANDARD;
55-
}
56-
}
5739
}

‎core/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.ast.statement;
7+
8+
import java.util.Locale;
9+
import lombok.Getter;
10+
import lombok.RequiredArgsConstructor;
11+
12+
@RequiredArgsConstructor
13+
public enum ExplainMode {
14+
SIMPLE("simple"),
15+
STANDARD("standard"),
16+
EXTENDED("extended"),
17+
COST("cost");
18+
19+
@Getter private final String modeName;
20+
21+
public static ExplainMode of(String mode) {
22+
if (mode == null || mode.isEmpty()) return STANDARD;
23+
try {
24+
return ExplainMode.valueOf(mode.toUpperCase(Locale.ROOT));
25+
} catch (Exception e) {
26+
return ExplainMode.STANDARD;
27+
}
28+
}
29+
}

‎core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
import lombok.RequiredArgsConstructor;
1414
import lombok.ToString;
1515
import org.apache.calcite.rel.RelNode;
16-
import org.opensearch.sql.ast.statement.Explain;
16+
import org.opensearch.sql.ast.statement.ExplainMode;
1717
import org.opensearch.sql.calcite.CalcitePlanContext;
1818
import org.opensearch.sql.common.response.ResponseListener;
1919
import org.opensearch.sql.data.model.ExprValue;
@@ -53,7 +53,7 @@ default void execute(
5353

5454
default void explain(
5555
RelNode plan,
56-
Explain.ExplainFormat format,
56+
ExplainMode mode,
5757
CalcitePlanContext context,
5858
ResponseListener<ExplainResponse> listener) {}
5959

‎core/src/main/java/org/opensearch/sql/executor/QueryService.java‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
import org.apache.calcite.tools.Programs;
2929
import org.opensearch.sql.analysis.AnalysisContext;
3030
import org.opensearch.sql.analysis.Analyzer;
31-
import org.opensearch.sql.ast.statement.Explain;
31+
import org.opensearch.sql.ast.statement.ExplainMode;
3232
import org.opensearch.sql.ast.tree.UnresolvedPlan;
3333
import org.opensearch.sql.calcite.CalcitePlanContext;
3434
import org.opensearch.sql.calcite.CalciteRelNodeVisitor;
@@ -83,11 +83,11 @@ public void explain(
8383
UnresolvedPlan plan,
8484
QueryType queryType,
8585
ResponseListener<ExecutionEngine.ExplainResponse> listener,
86-
Explain.ExplainFormat format) {
86+
ExplainMode mode) {
8787
if (shouldUseCalcite(queryType)) {
88-
explainWithCalcite(plan, queryType, listener, format);
88+
explainWithCalcite(plan, queryType, listener, mode);
8989
} else {
90-
explainWithLegacy(plan, queryType, listener, format, Optional.empty());
90+
explainWithLegacy(plan, queryType, listener, mode, Optional.empty());
9191
}
9292
}
9393

@@ -143,7 +143,7 @@ public void explainWithCalcite(
143143
UnresolvedPlan plan,
144144
QueryType queryType,
145145
ResponseListener<ExecutionEngine.ExplainResponse> listener,
146-
Explain.ExplainFormat format) {
146+
ExplainMode mode) {
147147
CalcitePlanContext.run(
148148
() -> {
149149
try {
@@ -160,15 +160,15 @@ public void explainWithCalcite(
160160

161161
RelNode calcitePlan = convertToCalcitePlan(relNode, context);
162162

163-
executionEngine.explain(calcitePlan, format, context, listener);
163+
executionEngine.explain(calcitePlan, mode, context, listener);
164164
},
165165
settings);
166166
return null;
167167
});
168168
} catch (Throwable t) {
169169
if (isCalciteFallbackAllowed(t)) {
170170
log.warn("Fallback to V2 query engine since got exception", t);
171-
explainWithLegacy(plan, queryType, listener, format, Optional.of(t));
171+
explainWithLegacy(plan, queryType, listener, mode, Optional.of(t));
172172
} else {
173173
if (t instanceof Error) {
174174
// Calcite may throw AssertError during query execution.
@@ -214,13 +214,12 @@ public void explainWithLegacy(
214214
UnresolvedPlan plan,
215215
QueryType queryType,
216216
ResponseListener<ExecutionEngine.ExplainResponse> listener,
217-
Explain.ExplainFormat format,
217+
ExplainMode mode,
218218
Optional<Throwable> calciteFailure) {
219219
try {
220-
if (format != null
221-
&& (format != Explain.ExplainFormat.STANDARD && format != Explain.ExplainFormat.YAML)) {
220+
if (mode != null && (mode != ExplainMode.STANDARD)) {
222221
throw new UnsupportedOperationException(
223-
"Explain mode " + format.name() + " is not supported in v2 engine");
222+
"Explain mode " + mode.name() + " is not supported in v2 engine");
224223
}
225224
executionEngine.explain(plan(analyze(plan, queryType)), listener);
226225
} catch (Exception e) {

‎core/src/main/java/org/opensearch/sql/executor/execution/AbstractPlan.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
import lombok.Getter;
1212
import lombok.RequiredArgsConstructor;
13-
import org.opensearch.sql.ast.statement.Explain;
13+
import org.opensearch.sql.ast.statement.ExplainMode;
1414
import org.opensearch.sql.common.response.ResponseListener;
1515
import org.opensearch.sql.executor.ExecutionEngine;
1616
import org.opensearch.sql.executor.QueryId;
@@ -34,5 +34,5 @@ public abstract class AbstractPlan {
3434
* @param listener query explain response listener.
3535
*/
3636
public abstract void explain(
37-
ResponseListener<ExecutionEngine.ExplainResponse> listener, Explain.ExplainFormat format);
37+
ResponseListener<ExecutionEngine.ExplainResponse> listener, ExplainMode mode);
3838
}

‎core/src/main/java/org/opensearch/sql/executor/execution/CommandPlan.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
package org.opensearch.sql.executor.execution;
1010

11-
import org.opensearch.sql.ast.statement.Explain;
11+
import org.opensearch.sql.ast.statement.ExplainMode;
1212
import org.opensearch.sql.ast.tree.UnresolvedPlan;
1313
import org.opensearch.sql.common.response.ResponseListener;
1414
import org.opensearch.sql.executor.ExecutionEngine;
@@ -50,7 +50,7 @@ public void execute() {
5050

5151
@Override
5252
public void explain(
53-
ResponseListener<ExecutionEngine.ExplainResponse> listener, Explain.ExplainFormat format) {
53+
ResponseListener<ExecutionEngine.ExplainResponse> listener, ExplainMode mode) {
5454
throw new UnsupportedOperationException("CommandPlan does not support explain");
5555
}
5656
}

‎core/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
package org.opensearch.sql.executor.execution;
1010

11-
import org.opensearch.sql.ast.statement.Explain;
11+
import org.opensearch.sql.ast.statement.ExplainMode;
1212
import org.opensearch.sql.common.response.ResponseListener;
1313
import org.opensearch.sql.executor.ExecutionEngine;
1414
import org.opensearch.sql.executor.QueryId;
@@ -18,7 +18,7 @@
1818
public class ExplainPlan extends AbstractPlan {
1919

2020
private final AbstractPlan plan;
21-
private final Explain.ExplainFormat format;
21+
private final ExplainMode mode;
2222

2323
private final ResponseListener<ExecutionEngine.ExplainResponse> explainListener;
2424

@@ -27,22 +27,22 @@ public ExplainPlan(
2727
QueryId queryId,
2828
QueryType queryType,
2929
AbstractPlan plan,
30-
Explain.ExplainFormat format,
30+
ExplainMode mode,
3131
ResponseListener<ExecutionEngine.ExplainResponse> explainListener) {
3232
super(queryId, queryType);
3333
this.plan = plan;
34-
this.format = format;
34+
this.mode = mode;
3535
this.explainListener = explainListener;
3636
}
3737

3838
@Override
3939
public void execute() {
40-
plan.explain(explainListener, format);
40+
plan.explain(explainListener, mode);
4141
}
4242

4343
@Override
4444
public void explain(
45-
ResponseListener<ExecutionEngine.ExplainResponse> listener, Explain.ExplainFormat format) {
45+
ResponseListener<ExecutionEngine.ExplainResponse> listener, ExplainMode mode) {
4646
throw new UnsupportedOperationException("explain query can not been explained.");
4747
}
4848
}

‎core/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
import java.util.Optional;
1212
import org.apache.commons.lang3.NotImplementedException;
13-
import org.opensearch.sql.ast.statement.Explain;
13+
import org.opensearch.sql.ast.statement.ExplainMode;
1414
import org.opensearch.sql.ast.tree.Paginate;
1515
import org.opensearch.sql.ast.tree.UnresolvedPlan;
1616
import org.opensearch.sql.common.response.ResponseListener;
@@ -72,13 +72,13 @@ public void execute() {
7272

7373
@Override
7474
public void explain(
75-
ResponseListener<ExecutionEngine.ExplainResponse> listener, Explain.ExplainFormat format) {
75+
ResponseListener<ExecutionEngine.ExplainResponse> listener, ExplainMode mode) {
7676
if (pageSize.isPresent()) {
7777
listener.onFailure(
7878
new NotImplementedException(
7979
"`explain` feature for paginated requests is not implemented yet."));
8080
} else {
81-
queryService.explain(plan, getQueryType(), listener, format);
81+
queryService.explain(plan, getQueryType(), listener, mode);
8282
}
8383
}
8484
}

‎core/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.apache.commons.lang3.tuple.Pair;
1515
import org.opensearch.sql.ast.AbstractNodeVisitor;
1616
import org.opensearch.sql.ast.statement.Explain;
17+
import org.opensearch.sql.ast.statement.ExplainMode;
1718
import org.opensearch.sql.ast.statement.Query;
1819
import org.opensearch.sql.ast.statement.Statement;
1920
import org.opensearch.sql.ast.tree.CloseCursor;
@@ -80,7 +81,7 @@ public AbstractPlan create(
8081
new QueryPlan(
8182
queryId, queryType, new FetchCursor(cursor), queryService, queryResponseListener);
8283
return isExplain
83-
? new ExplainPlan(queryId, queryType, plan, Explain.format(format), explainListener)
84+
? new ExplainPlan(queryId, queryType, plan, ExplainMode.of(format), explainListener)
8485
: plan;
8586
}
8687

@@ -140,7 +141,7 @@ public AbstractPlan visitExplain(
140141
QueryId.queryId(),
141142
node.getQueryType(),
142143
create(node.getStatement(), NO_CONSUMER_RESPONSE_LISTENER, context.getRight()),
143-
node.getFormat(),
144+
node.getMode(),
144145
context.getRight());
145146
}
146147
}

‎core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
import org.mockito.Mock;
2424
import org.mockito.junit.jupiter.MockitoExtension;
2525
import org.opensearch.sql.analysis.Analyzer;
26-
import org.opensearch.sql.ast.statement.Explain;
26+
import org.opensearch.sql.ast.statement.ExplainMode;
2727
import org.opensearch.sql.ast.tree.UnresolvedPlan;
2828
import org.opensearch.sql.common.response.ResponseListener;
2929
import org.opensearch.sql.common.setting.Settings;
@@ -62,7 +62,7 @@ class QueryServiceTest {
6262

6363
@Mock private Split split;
6464

65-
private final Explain.ExplainFormat format = Explain.ExplainFormat.STANDARD;
65+
private final ExplainMode mode = ExplainMode.STANDARD;
6666

6767
@Test
6868
public void executeWithoutContext() {
@@ -225,7 +225,7 @@ public void onFailure(Exception e) {
225225
fail();
226226
}
227227
},
228-
format);
228+
mode);
229229
}
230230

231231
void handledByExplainOnFailure() {
@@ -243,7 +243,7 @@ public void onFailure(Exception e) {
243243
assertTrue(e instanceof IllegalStateException);
244244
}
245245
},
246-
format);
246+
mode);
247247
}
248248
}
249249
}

0 commit comments

Comments
 (0)