KurtYoung commented on a change in pull request #15366:
URL: https://github.com/apache/flink/pull/15366#discussion_r602684056
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java
##########
@@ -106,9 +111,28 @@ private void openCli(String sessionId, Executor executor) {
SystemUtils.IS_OS_WINDOWS ? "flink-sql-history" :
".flink-sql-history");
}
- try (CliClient cli = new CliClient(sessionId, executor,
historyFilePath)) {
- // interactive CLI mode
- if (options.getUpdateStatement() == null) {
+ boolean isInteractiveMode = options.getSqlFile() == null;
+ boolean hasUpdateStatement = options.getUpdateStatement() != null;
+ if (!isInteractiveMode && hasUpdateStatement) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Please use either option %s or %s. The option %s
is deprecated and it's suggested to use %s instead.",
+ CliOptionsParser.OPTION_FILE,
+ CliOptionsParser.OPTION_UPDATE,
+ CliOptionsParser.OPTION_UPDATE.getOpt(),
+ CliOptionsParser.OPTION_FILE.getOpt()));
+ }
+
+ try (CliClient cli =
+ isInteractiveMode
+ ? CliClient.createInteractiveClient(sessionId,
executor, historyFilePath)
Review comment:
If user specify `-u` only, this will create an interactive client, which
I think is not you want
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java
##########
@@ -106,9 +111,28 @@ private void openCli(String sessionId, Executor executor) {
SystemUtils.IS_OS_WINDOWS ? "flink-sql-history" :
".flink-sql-history");
}
- try (CliClient cli = new CliClient(sessionId, executor,
historyFilePath)) {
- // interactive CLI mode
- if (options.getUpdateStatement() == null) {
+ boolean isInteractiveMode = options.getSqlFile() == null;
+ boolean hasUpdateStatement = options.getUpdateStatement() != null;
+ if (!isInteractiveMode && hasUpdateStatement) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Please use either option %s or %s. The option %s
is deprecated and it's suggested to use %s instead.",
+ CliOptionsParser.OPTION_FILE,
+ CliOptionsParser.OPTION_UPDATE,
+ CliOptionsParser.OPTION_UPDATE.getOpt(),
+ CliOptionsParser.OPTION_FILE.getOpt()));
+ }
+
+ try (CliClient cli =
+ isInteractiveMode
+ ? CliClient.createInteractiveClient(sessionId,
executor, historyFilePath)
+ : CliClient.createNonInteractiveClient(
+ sessionId,
+ executor,
+ historyFilePath,
+ readFromURL(options.getSqlFile()),
+ null)) {
+ if (!hasUpdateStatement) {
Review comment:
How about we unifying the logic to dealing with `-u` and `-f` by passing
the `-u` content the same way as `-f` does.
This will implicitly expand the functionality of `-u` (it only accepts DML
before but now support much more), but consider it as deprecated and we are
about to delete it, I think it's worth to make the code clean now.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java
##########
@@ -195,4 +219,12 @@ public void run() {
System.out.println("done.");
}
}
+
+ private String readFromURL(URL file) {
+ try {
+ return IOUtils.toString(file, StandardCharsets.UTF_8);
+ } catch (IOException e) {
+ throw new SqlExecutionException("Work work work", e);
Review comment:
what does this error message mean?
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java
##########
@@ -106,9 +111,28 @@ private void openCli(String sessionId, Executor executor) {
SystemUtils.IS_OS_WINDOWS ? "flink-sql-history" :
".flink-sql-history");
}
- try (CliClient cli = new CliClient(sessionId, executor,
historyFilePath)) {
- // interactive CLI mode
- if (options.getUpdateStatement() == null) {
+ boolean isInteractiveMode = options.getSqlFile() == null;
Review comment:
These logic is kind of messy and error prone.
IIUC `isInteractiveMode` should be true only when `-f` and `-u` are both
null.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -121,6 +135,7 @@ public CliClient(
.appName(CliStrings.CLI_NAME)
.parser(new SqlMultiLineParser())
.completer(new SqlCompleter(sessionId, executor))
+ .variable(LineReader.COMMENT_BEGIN, COMMENT_BEGIN)
Review comment:
Could you set this variable with `lineReader.setVariable(....);` and add
some comments to explain why you set this?
##########
File path:
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientTest.java
##########
@@ -129,6 +142,126 @@ public void testHistoryFile() throws Exception {
}
}
+ @Test
+ public void testSqlFileContainsUnknownStatement() throws Exception {
Review comment:
Also check the error message here. I tried an invalid command and the
error message seems difficult to understand:
```
Flink SQL> invalid commond;
[ERROR] Could not execute SQL statement. Reason:
org.apache.calcite.runtime.CalciteException: Non-query expression encountered
in illegal context
```
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -230,8 +242,15 @@ public void open() {
if (line == null) {
continue;
}
+
final Optional<Operation> operation = parseCommand(line);
- operation.ifPresent(this::callOperation);
+ if (operation.isPresent()) {
+ boolean success = callOperation(operation.get());
Review comment:
How about letting all the `callXXXX` methods throws
`SqlExecutionException` instead of returning flag?
I noticed the error handling logic in every `callXXXX` is the same:
```
catch (SqlExecutionException e) {
printExecutionException(e);
return false;
}
```
Thus I think it would be also enough to handle the exception here, and you
can check whether it's interactive mode and do things you want.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -220,6 +229,9 @@ public void open() {
line = lineReader.readLine(prompt, null, inputTransformer,
null);
} catch (UserInterruptException e) {
// user cancelled line with Ctrl+C
+ if (!isInteractive) {
Review comment:
Add a test case about this. The logic we want to verify is:
1. If user types CTRL-C in interactive mode, it only cancelled one line
2. If user types CTRL-C in non-interactive mode, it cancelled the whole CLI.
--
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:
[email protected]
