LadyForest commented on code in PR #22515:
URL: https://github.com/apache/flink/pull/22515#discussion_r1184990962
##########
flink-table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkSqlParserImplTest.java:
##########
@@ -2072,6 +2072,13 @@ void testExplainPlanFor() {
this.sql(sql).ok(expected);
}
+ @Test
+ void testExplainForJsonFile() {
+ String sql = "explain plan for 'file://path'";
+ String expected = "EXPLAIN 'file://path'";
Review Comment:
Please add more cases to cover `ExplainDetail`s, e.g.
```java
this.sql("explain changelog_mode '/mydir/plan.json'").ok(EXPLAIN
CHANGELOG_MODE '/mydir/plan.json');
```
##########
flink-table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkSqlParserImplTest.java:
##########
@@ -2072,6 +2072,13 @@ void testExplainPlanFor() {
this.sql(sql).ok(expected);
}
+ @Test
+ void testExplainForJsonFile() {
Review Comment:
nit: `testExplainCompiledPlan`
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentImpl.java:
##########
@@ -915,6 +916,16 @@ public TableResultInternal executeInternal(Operation
operation) {
return executeInternal(((StatementSetOperation)
operation).getOperations());
} else if (operation instanceof ExplainOperation) {
ExplainOperation explainOperation = (ExplainOperation) operation;
+ if (explainOperation instanceof ExplainFileOperation) {
+ ExplainFileOperation explainfileOperation =
(ExplainFileOperation) explainOperation;
+ CompiledPlan cp =
+
loadPlan(PlanReference.fromFile(explainfileOperation.getFilePath()));
Review Comment:
Why just load the plan and return it? I think we should call
`compiledPlan#explain(...)`
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dql/SqlRichExplain.java:
##########
@@ -58,6 +60,10 @@ public SqlNode getStatement() {
return statement;
}
+ public String getPlanFile() {
Review Comment:
It might be weird to add this method here. `SqlRichExplain` is a generalized
representation of the syntax, while `getPlanFile` is only suitable for `EXPLAIN
'plan.json'`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
