KurtYoung commented on a change in pull request #15437:
URL: https://github.com/apache/flink/pull/15437#discussion_r604538126
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java
##########
@@ -111,6 +111,7 @@ private void openCli(String sessionId, Executor executor) {
SystemUtils.IS_OS_WINDOWS ? "flink-sql-history" :
".flink-sql-history");
}
+ boolean hasInitFile = options.getInitFile() != null;
Review comment:
Move variable definition as close as to the usage
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java
##########
@@ -124,6 +125,22 @@ private void openCli(String sessionId, Executor executor) {
}
boolean isInteractiveMode = !hasSqlFile && !hasUpdateStatement;
+ if (hasInitFile) {
+ boolean success =
+ CliClient.initializeSession(
+ sessionId,
+ executor,
+ historyFilePath,
+ readFromURL(options.getInitFile()));
+ if (!success) {
+ // stop execution if initialization fails.
Review comment:
useless comment
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java
##########
@@ -124,6 +125,22 @@ private void openCli(String sessionId, Executor executor) {
}
boolean isInteractiveMode = !hasSqlFile && !hasUpdateStatement;
Review comment:
Define the variable as close as to the usage is a good coding practice.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java
##########
@@ -111,6 +111,7 @@ private void openCli(String sessionId, Executor executor) {
SystemUtils.IS_OS_WINDOWS ? "flink-sql-history" :
".flink-sql-history");
}
+ boolean hasInitFile = options.getInitFile() != null;
Review comment:
Actually I don't think you need this local variable
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -297,17 +310,23 @@ private boolean executeStatement(String statement,
ExecutionMode executionMode)
}
private void validate(Operation operation, ExecutionMode executionMode) {
- ResultMode mode =
executor.getSessionConfig(sessionId).get(EXECUTION_RESULT_MODE);
- if (operation instanceof QueryOperation
- && executionMode.equals(ExecutionMode.NON_INTERACTIVE)
- && !mode.equals(TABLEAU)) {
- throw new IllegalArgumentException(
- String.format(
- "In non-interactive mode, it only supports to use
%s as value of %s when execute query. Please add 'SET %s=%s;' in the sql file.",
- TABLEAU,
- EXECUTION_RESULT_MODE.key(),
- EXECUTION_RESULT_MODE.key(),
- TABLEAU));
+ if (executionMode.equals(ExecutionMode.INITIALIZATION)) {
+ if (operation instanceof QueryOperation || operation instanceof
ModifyOperation) {
Review comment:
I suggest you list all the operations you support in init file instead
of listing the ones you don't support.
This will prevent you leaking some newly introduced operations such
`StatementSetOperation`.
AFAIK `StatementSetOperation` shouldn't be support in this init file.
And for operations like `explain`, I also don't think it's meaningful to
support it in init file
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java
##########
@@ -124,6 +125,22 @@ private void openCli(String sessionId, Executor executor) {
}
boolean isInteractiveMode = !hasSqlFile && !hasUpdateStatement;
+ if (hasInitFile) {
+ boolean success =
+ CliClient.initializeSession(
Review comment:
Why init client here instead of after you created the real cli client?
--
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
