LadyForest commented on a change in pull request #15005:
URL: https://github.com/apache/flink/pull/15005#discussion_r582802303
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/local/LocalExecutor.java
##########
@@ -428,6 +429,13 @@ public ProgramTargetDescriptor executeUpdate(String
sessionId, String statement)
return executeUpdateInternal(sessionId, context, statement);
}
+ @VisibleForTesting
+ List<ModuleEntry> listFullModules(String sessionId) throws
SqlExecutionException {
Review comment:
Hi Jark, I've also considered this point before issuing this PR.
I thought the reason we have this method `LocalExecutor#listModules()` is
because the `SHOW MODULES` syntax is not supported by the
`FlinkSqlParserImpl`and `TableEnvironment` at that moment. Thus we cannot
perform `localExecutor.executeSql("SHOW MODULES")`.
Once the `SHOW [FULL] MODULES` syntax is supported, all `SHOW MODULES`
commands would go for the table result (except for `SHOW MODULES` using the old
planner). That's why I didn't make `listFullModules()` an interface method. I
was meant to make it a testing method.
Towards the next PR, since `SHOW FULL MODULES` will display the module's use
status, `TableSchema` is needed for the result and print as tableau form. The
extra effort will be put in if calling `localExecutor.listFullModules()`
directly. However, we can reuse `PrintUtils#printAsTableauForm` if using
`localExecutor.executeSql("SHOW FULL MODULES")`.
Please correct me if I miss something or not understand wrong:-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
