Skip to content

Commit 5dbcd28

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

41 files changed

Lines changed: 523 additions & 153 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
@@ -5,7 +5,6 @@
55

66
package org.opensearch.sql.ast.statement;
77

8-
import java.util.Locale;
98
import lombok.EqualsAndHashCode;
109
import lombok.Getter;
1110
import org.opensearch.sql.ast.AbstractNodeVisitor;
@@ -18,37 +17,20 @@ public class Explain extends Statement {
1817

1918
private final Statement statement;
2019
private final QueryType queryType;
21-
private final ExplainFormat format;
20+
private final ExplainMode mode;
2221

2322
public Explain(Statement statement, QueryType queryType) {
2423
this(statement, queryType, null);
2524
}
2625

27-
public Explain(Statement statement, QueryType queryType, String format) {
26+
public Explain(Statement statement, QueryType queryType, String mode) {
2827
this.statement = statement;
2928
this.queryType = queryType;
30-
this.format = Explain.format(format);
29+
this.mode = ExplainMode.of(mode);
3130
}
3231

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

‎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
@@ -26,7 +26,7 @@
2626
import org.apache.calcite.tools.Programs;
2727
import org.opensearch.sql.analysis.AnalysisContext;
2828
import org.opensearch.sql.analysis.Analyzer;
29-
import org.opensearch.sql.ast.statement.Explain;
29+
import org.opensearch.sql.ast.statement.ExplainMode;
3030
import org.opensearch.sql.ast.tree.UnresolvedPlan;
3131
import org.opensearch.sql.calcite.CalcitePlanContext;
3232
import org.opensearch.sql.calcite.CalciteRelNodeVisitor;
@@ -81,11 +81,11 @@ public void explain(
8181
UnresolvedPlan plan,
8282
QueryType queryType,
8383
ResponseListener<ExecutionEngine.ExplainResponse> listener,
84-
Explain.ExplainFormat format) {
84+
ExplainMode mode) {
8585
if (shouldUseCalcite(queryType)) {
86-
explainWithCalcite(plan, queryType, listener, format);
86+
explainWithCalcite(plan, queryType, listener, mode);
8787
} else {
88-
explainWithLegacy(plan, queryType, listener, format, Optional.empty());
88+
explainWithLegacy(plan, queryType, listener, mode, Optional.empty());
8989
}
9090
}
9191

@@ -134,7 +134,7 @@ public void explainWithCalcite(
134134
UnresolvedPlan plan,
135135
QueryType queryType,
136136
ResponseListener<ExecutionEngine.ExplainResponse> listener,
137-
Explain.ExplainFormat format) {
137+
ExplainMode mode) {
138138
CalcitePlanContext.run(
139139
() -> {
140140
try {
@@ -146,13 +146,13 @@ public void explainWithCalcite(
146146
() -> {
147147
RelNode relNode = analyze(plan, context);
148148
RelNode calcitePlan = convertToCalcitePlan(relNode, context);
149-
executionEngine.explain(calcitePlan, format, context, listener);
149+
executionEngine.explain(calcitePlan, mode, context, listener);
150150
},
151151
settings);
152152
} catch (Throwable t) {
153153
if (isCalciteFallbackAllowed(t)) {
154154
log.warn("Fallback to V2 query engine since got exception", t);
155-
explainWithLegacy(plan, queryType, listener, format, Optional.of(t));
155+
explainWithLegacy(plan, queryType, listener, mode, Optional.of(t));
156156
} else {
157157
if (t instanceof Error) {
158158
// Calcite may throw AssertError during query execution.
@@ -198,13 +198,12 @@ public void explainWithLegacy(
198198
UnresolvedPlan plan,
199199
QueryType queryType,
200200
ResponseListener<ExecutionEngine.ExplainResponse> listener,
201-
Explain.ExplainFormat format,
201+
ExplainMode mode,
202202
Optional<Throwable> calciteFailure) {
203203
try {
204-
if (format != null
205-
&& (format != Explain.ExplainFormat.STANDARD && format != Explain.ExplainFormat.YAML)) {
204+
if (mode != null && (mode != ExplainMode.STANDARD)) {
206205
throw new UnsupportedOperationException(
207-
"Explain mode " + format.name() + " is not supported in v2 engine");
206+
"Explain mode " + mode.name() + " is not supported in v2 engine");
208207
}
209208
executionEngine.explain(plan(analyze(plan, queryType)), listener);
210209
} 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
@@ -7,7 +7,7 @@
77

88
import lombok.Getter;
99
import lombok.RequiredArgsConstructor;
10-
import org.opensearch.sql.ast.statement.Explain;
10+
import org.opensearch.sql.ast.statement.ExplainMode;
1111
import org.opensearch.sql.common.response.ResponseListener;
1212
import org.opensearch.sql.executor.ExecutionEngine;
1313
import org.opensearch.sql.executor.QueryId;
@@ -31,5 +31,5 @@ public abstract class AbstractPlan {
3131
* @param listener query explain response listener.
3232
*/
3333
public abstract void explain(
34-
ResponseListener<ExecutionEngine.ExplainResponse> listener, Explain.ExplainFormat format);
34+
ResponseListener<ExecutionEngine.ExplainResponse> listener, ExplainMode mode);
3535
}

‎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
@@ -5,7 +5,7 @@
55

66
package org.opensearch.sql.executor.execution;
77

8-
import org.opensearch.sql.ast.statement.Explain;
8+
import org.opensearch.sql.ast.statement.ExplainMode;
99
import org.opensearch.sql.ast.tree.UnresolvedPlan;
1010
import org.opensearch.sql.common.response.ResponseListener;
1111
import org.opensearch.sql.executor.ExecutionEngine;
@@ -47,7 +47,7 @@ public void execute() {
4747

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

‎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
@@ -5,7 +5,7 @@
55

66
package org.opensearch.sql.executor.execution;
77

8-
import org.opensearch.sql.ast.statement.Explain;
8+
import org.opensearch.sql.ast.statement.ExplainMode;
99
import org.opensearch.sql.common.response.ResponseListener;
1010
import org.opensearch.sql.executor.ExecutionEngine;
1111
import org.opensearch.sql.executor.QueryId;
@@ -15,7 +15,7 @@
1515
public class ExplainPlan extends AbstractPlan {
1616

1717
private final AbstractPlan plan;
18-
private final Explain.ExplainFormat format;
18+
private final ExplainMode mode;
1919

2020
private final ResponseListener<ExecutionEngine.ExplainResponse> explainListener;
2121

@@ -24,22 +24,22 @@ public ExplainPlan(
2424
QueryId queryId,
2525
QueryType queryType,
2626
AbstractPlan plan,
27-
Explain.ExplainFormat format,
27+
ExplainMode mode,
2828
ResponseListener<ExecutionEngine.ExplainResponse> explainListener) {
2929
super(queryId, queryType);
3030
this.plan = plan;
31-
this.format = format;
31+
this.mode = mode;
3232
this.explainListener = explainListener;
3333
}
3434

3535
@Override
3636
public void execute() {
37-
plan.explain(explainListener, format);
37+
plan.explain(explainListener, mode);
3838
}
3939

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

‎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
@@ -7,7 +7,7 @@
77

88
import java.util.Optional;
99
import org.apache.commons.lang3.NotImplementedException;
10-
import org.opensearch.sql.ast.statement.Explain;
10+
import org.opensearch.sql.ast.statement.ExplainMode;
1111
import org.opensearch.sql.ast.tree.Paginate;
1212
import org.opensearch.sql.ast.tree.UnresolvedPlan;
1313
import org.opensearch.sql.common.response.ResponseListener;
@@ -69,13 +69,13 @@ public void execute() {
6969

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

‎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
@@ -11,6 +11,7 @@
1111
import org.apache.commons.lang3.tuple.Pair;
1212
import org.opensearch.sql.ast.AbstractNodeVisitor;
1313
import org.opensearch.sql.ast.statement.Explain;
14+
import org.opensearch.sql.ast.statement.ExplainMode;
1415
import org.opensearch.sql.ast.statement.Query;
1516
import org.opensearch.sql.ast.statement.Statement;
1617
import org.opensearch.sql.ast.tree.CloseCursor;
@@ -77,7 +78,7 @@ public AbstractPlan create(
7778
new QueryPlan(
7879
queryId, queryType, new FetchCursor(cursor), queryService, queryResponseListener);
7980
return isExplain
80-
? new ExplainPlan(queryId, queryType, plan, Explain.format(format), explainListener)
81+
? new ExplainPlan(queryId, queryType, plan, ExplainMode.of(format), explainListener)
8182
: plan;
8283
}
8384

@@ -137,7 +138,7 @@ public AbstractPlan visitExplain(
137138
QueryId.queryId(),
138139
node.getQueryType(),
139140
create(node.getStatement(), NO_CONSUMER_RESPONSE_LISTENER, context.getRight()),
140-
node.getFormat(),
141+
node.getMode(),
141142
context.getRight());
142143
}
143144
}

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
import org.mockito.Mock;
2121
import org.mockito.junit.jupiter.MockitoExtension;
2222
import org.opensearch.sql.analysis.Analyzer;
23-
import org.opensearch.sql.ast.statement.Explain;
23+
import org.opensearch.sql.ast.statement.ExplainMode;
2424
import org.opensearch.sql.ast.tree.UnresolvedPlan;
2525
import org.opensearch.sql.common.response.ResponseListener;
2626
import org.opensearch.sql.common.setting.Settings;
@@ -59,7 +59,7 @@ class QueryServiceTest {
5959

6060
@Mock private Split split;
6161

62-
private final Explain.ExplainFormat format = Explain.ExplainFormat.STANDARD;
62+
private final ExplainMode mode = ExplainMode.STANDARD;
6363

6464
@Test
6565
public void executeWithoutContext() {
@@ -222,7 +222,7 @@ public void onFailure(Exception e) {
222222
fail();
223223
}
224224
},
225-
format);
225+
mode);
226226
}
227227

228228
void handledByExplainOnFailure() {
@@ -240,7 +240,7 @@ public void onFailure(Exception e) {
240240
assertTrue(e instanceof IllegalStateException);
241241
}
242242
},
243-
format);
243+
mode);
244244
}
245245
}
246246
}

0 commit comments

Comments
 (0)