wuchong commented on a change in pull request #15534:
URL: https://github.com/apache/flink/pull/15534#discussion_r614726916
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -648,7 +647,7 @@ private LineReader createLineReader(Terminal terminal) {
if (Files.exists(historyFilePath) ||
CliUtils.createFile(historyFilePath)) {
String msg = "Command history file path: " + historyFilePath;
// print it in the command line as well as log file
- System.out.println(msg);
+ terminal.writer().println(msg);
LOG.info(msg);
lineReader.setVariable(LineReader.HISTORY_FILE, historyFilePath);
} else {
Review comment:
Should we also use `terminal.writer()` in the `else` branch?
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/TerminalUtils.java
##########
@@ -47,16 +47,17 @@ public static Terminal createDummyTerminal(OutputStream
out) {
}
}
- public static Terminal createDefaultTerminal(boolean useSystemInOutStream)
{
+ public static Terminal createDummyTerminal(InputStream in, OutputStream
out) {
Review comment:
This is not a dummy terminal, but a Dumb terminal.
##########
File path:
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientTest.java
##########
@@ -356,12 +352,14 @@ private String executeSqlFromContent(MockExecutor
executor, String content) thro
OutputStream outputStream = new ByteArrayOutputStream(256);
try (CliClient client =
new CliClient(
- TerminalUtils.createDummyTerminal(outputStream),
+ () -> TerminalUtils.createDummyTerminal(outputStream),
sessionId,
executor,
historyTempFile(),
null)) {
- client.executeFile(content,
CliClient.ExecutionMode.NON_INTERACTIVE_EXECUTION);
+ // client.executeFile(content,
+ // CliClient.ExecutionMode.NON_INTERACTIVE_EXECUTION);
Review comment:
remove
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java
##########
@@ -151,6 +153,11 @@ private void openCli(String sessionId, Executor executor) {
//
--------------------------------------------------------------------------------------------
public static void main(String[] args) {
+ startClient(args, SqlClient::new);
+ }
+
+ protected static void startClient(
+ String[] args, BiFunction<Boolean, CliOptions, SqlClient>
clientCreator) {
Review comment:
Usually, we call such thing "factory".
##########
File path:
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/SqlClientTest.java
##########
@@ -218,26 +186,44 @@ public void testInitFile() throws IOException {
String initFile = createSqlFile(statements, "init-sql.sql");
String[] args = new String[] {"-i", initFile};
- SqlClient.main(args);
-
- assertThat(getStdoutString(), containsString("Successfully initialized
from sql script: "));
+ String output = runSqlClient(args, "SET;\nQUIT;\n");
+ assertThat(output, containsString("key=value"));
}
@Test
- public void testExecuteSqlFile() throws IOException {
- List<String> statements = Collections.singletonList("HELP;");
+ public void testExecuteSqlFile() throws Exception {
+ List<String> statements = Collections.singletonList("HELP;\n");
String sqlFilePath = createSqlFile(statements, "test-sql.sql");
String[] args = new String[] {"-f", sqlFilePath};
- SqlClient.main(args);
+ String output = runSqlClient(args);
final URL url =
getClass().getClassLoader().getResource("sql-client-help-command.out");
final String help = FileUtils.readFileUtf8(new File(url.getFile()));
- final String output = getStdoutString();
for (String command : help.split("\n")) {
assertThat(output, containsString(command));
}
}
+ private String runSqlClient(String[] args) throws Exception {
+ return runSqlClient(args, "QUIT;\n");
+ }
+
+ private String runSqlClient(String[] args, String statements) throws
Exception {
+ try (OutputStream out = new ByteArrayOutputStream();
+ Terminal terminal =
+ TerminalUtils.createDummyTerminal(
+ new ByteArrayInputStream(
+
statements.getBytes(StandardCharsets.UTF_8)),
+ out)) {
+ MockedSqlClient.mockedMain(args, terminal);
+ return getStdoutString(out);
+ }
+ }
+
+ private String getStdoutString(OutputStream out) {
Review comment:
We don't need extract a method for such a simple line.
##########
File path:
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/SqlClientTest.java
##########
@@ -218,26 +186,44 @@ public void testInitFile() throws IOException {
String initFile = createSqlFile(statements, "init-sql.sql");
String[] args = new String[] {"-i", initFile};
- SqlClient.main(args);
-
- assertThat(getStdoutString(), containsString("Successfully initialized
from sql script: "));
+ String output = runSqlClient(args, "SET;\nQUIT;\n");
+ assertThat(output, containsString("key=value"));
Review comment:
Could you use `SET key = value;` instead of `SET key=value` in the init
file, to avoid the `containsString("key=value")` may match the input
statements? (I know the output doesn't contain input statements now)
--
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]
