wuchong commented on a change in pull request #15366:
URL: https://github.com/apache/flink/pull/15366#discussion_r603236144
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliOptions.java
##########
@@ -76,6 +79,10 @@ public URL getDefaults() {
return defaults;
}
+ public URL getSqlFile() {
Review comment:
Add `@Nullable` on the return type.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -212,31 +215,51 @@ public void open() {
isRunning = true;
// print welcome
- terminal.writer().append(CliStrings.MESSAGE_WELCOME);
+ if (isInteractive) {
Review comment:
This looks really hard to maintain which mixing interactive logic and
non-interactive logic. I suggest do not open terminal if in non-interactive
mode. We can simply throw exception here if in non-interactive mode. The`-f` or
`-u` or `-i` content can be execute by an new method, e.g.
`executeMultilineSql(String content)`.
##########
File path:
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/SqlClientTest.java
##########
@@ -89,6 +97,21 @@ public void before() throws IOException {
CommonTestUtils.setEnv(map);
historyPath = tempFolder.newFile("history").toString();
+
+ // create sql file
+ File sqlFileFolder = tempFolder.newFolder("sql-file");
+ File sqlFile = new File(sqlFileFolder, "test-sql.sql");
+ if (!sqlFile.createNewFile()) {
+ throw new IOException("Can't create testing test-sql.sql file.");
+ }
+ sqlFilePath = sqlFile.getPath();
+
+ List<String> statements = Collections.singletonList("HELP;");
Review comment:
Could you use some meaningful statements here? e.g. DDL + INSERT INTO +
SELECT.
The HELP command has been verified in `misc.q` and need to maintain every
time we update help output. I think it's not a good statement for verifying
`-f`.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/SqlMultiLineParser.java
##########
@@ -33,6 +35,7 @@
private static final String EOF_CHARACTER = ";";
private static final String NEW_LINE_PROMPT = ""; // results in simple '>'
output
+ private static final String MASK = "--.*$";
Review comment:
not used.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliOptionsParser.java
##########
@@ -107,18 +118,22 @@
+ "functions, table sources, or sinks. Can
be used multiple times.")
.build();
+ @Deprecated
public static final Option OPTION_UPDATE =
Option.builder("u")
.required(false)
.longOpt("update")
.numberOfArgs(1)
.argName("SQL update statement")
.desc(
- "Experimental (for testing only!): Instructs the
SQL Client to immediately execute "
- + "the given update statement after
starting up. The process is shut down after the "
- + "statement has been submitted to the
cluster and returns an appropriate return code. "
- + "Currently, this feature is only
supported for INSERT INTO statements that declare "
- + "the target sink table.")
+ String.format(
+ "Deprecated Experimental (for testing
only!) feature: Instructs the SQL Client to immediately execute "
+ + "the given update statement
after starting up. The process is shut down after the "
+ + "statement has been submitted to
the cluster and returns an appropriate return code. "
+ + "Currently, this feature is only
supported for INSERT INTO statements that declare "
+ + "the target sink table."
+ + "Please use option %s to submit
update statement.",
+ OPTION_FILE.getOpt()))
Review comment:
I'm afraid `OPTION_FILE.getOpt()` just print `f` instead of `-f`.
##########
File path:
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/SqlClientTest.java
##########
@@ -89,6 +97,21 @@ public void before() throws IOException {
CommonTestUtils.setEnv(map);
historyPath = tempFolder.newFile("history").toString();
+
+ // create sql file
+ File sqlFileFolder = tempFolder.newFolder("sql-file");
Review comment:
Not every tests need this, please move this to the test which needs
this.
##########
File path:
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientTest.java
##########
@@ -134,58 +142,113 @@ public void testHistoryFile() throws Exception {
}
@Test
- public void verifyCancelSubmitInSyncMode() throws Exception {
- final MockExecutor mockExecutor = new MockExecutor(true);
- String sessionId = mockExecutor.openSession("test-session");
+ public void testGetEOFinInteractiveMode() throws Exception {
+ final List<String> statements =
+ Arrays.asList("DESC MyOtherTable;", "SHOW TABLES"); // meet EOF
+ String content = String.join("\n", statements);
- ByteArrayOutputStream outputStream = new ByteArrayOutputStream(256);
-
- try (CliClient client =
- new CliClient(
- TerminalUtils.createDummyTerminal(outputStream),
- sessionId,
- mockExecutor,
- historyTempFile(),
- null)) {
- FutureTask<Boolean> task =
- new FutureTask<>(() ->
client.submitUpdate(INSERT_INTO_STATEMENT));
- Thread thread = new Thread(task);
- thread.start();
+ final MockExecutor mockExecutor = new MockExecutor();
- while (!mockExecutor.isAwait) {
- Thread.sleep(10);
- }
+ executeSqlFromContent(mockExecutor, content, true);
+ // don't execute the last commands
+ assertTrue(statements.get(0).contains(mockExecutor.receivedStatement));
+ }
- thread.interrupt();
- assertFalse(task.get());
- assertTrue(
- outputStream
- .toString()
- .contains("java.lang.InterruptedException: sleep
interrupted"));
- }
+ @Test
+ public void testGetEOFinNonInteractiveMode() throws Exception {
+ final List<String> statements =
+ Arrays.asList("DESC MyOtherTable;", "SHOW TABLES"); // meet EOF
Review comment:
We should throw exception for the last statement in `-f` mode to tell
users the SQL script is not valid. Otherwise, users will be surprised the last
statement is not executed.
--
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]
