wuchong commented on a change in pull request #15197:
URL: https://github.com/apache/flink/pull/15197#discussion_r595108991
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java
##########
@@ -180,6 +180,13 @@ private CliStrings() {
public static final String MESSAGE_SET = "Session property has been set.";
+ public static final String MESSAGE_SET_REMOVED_KEY =
+ "The specified key is not supported anymore.";
+
+ public static final String MESSAGE_SET_DEPRECATED_KEY =
+ "The specified key '%s' is deprecated. Please use '%s' instead. "
+ + "Set the corresponding config option and the specified
key together.";
Review comment:
Remove this line. It sounds like suggest users to set both key.
We will print `Session property has been set` which has tell users the key
is effective, so I think we don't need tell users both key will be set.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -453,22 +457,34 @@ private void callSet(SqlCommandCall cmdCall) {
terminal.writer()
.println(CliStrings.messageInfo(CliStrings.MESSAGE_EMPTY).toAnsi());
} else {
- properties.entrySet().stream()
- .map((e) -> e.getKey() + "=" + e.getValue())
- .sorted()
- .forEach((p) -> terminal.writer().println(p));
+ List<String> prettyEntries =
YamlConfigUtils.getPropertiesInPretty(properties);
+ prettyEntries.forEach(entry ->
terminal.writer().println(entry));
}
}
// set a property
else {
+ String key = cmdCall.operands[0].trim();
+ String value = cmdCall.operands[1].trim();
try {
- executor.setSessionProperty(
- sessionId, cmdCall.operands[0],
cmdCall.operands[1].trim());
+ executor.setSessionProperty(sessionId, key, value);
} catch (SqlExecutionException e) {
printExecutionException(e);
return;
}
-
terminal.writer().println(CliStrings.messageInfo(CliStrings.MESSAGE_SET).toAnsi());
+ if (YamlConfigUtils.isRemovedKey(key)) {
+
terminal.writer().println(CliStrings.messageWarning(MESSAGE_SET_REMOVED_KEY));
+ return;
Review comment:
I think this misses to flush the terminal.
I suggest to not return here, and move the `MESSAGE_SET` printing into the
`else` branch.
##########
File path: flink-table/flink-sql-client/src/test/resources/sql/set.q
##########
@@ -42,20 +42,11 @@ CREATE TABLE hive_table (
[INFO] Table has been created.
!info
-# list the configured configuration
-set;
-table.sql-dialect=hive
-!ok
-
Review comment:
I think we must resolve this problem. Otherwise, we can't never test
`SET` command. I have some ideas:
1) support dynamic variable in the `*.q` script, e.g. `$VAR_PIPELINE_JARS`,
and replace the varibles in `CliClientITCase` after read script file. Just like
what we do for the yaml files.
2) support `SET LIKE 'table.*'` syntax to just show some specific
configurations. But this need another issue to discuss and still can't test
`SET` command. But I think this can be a useful feature if the configuration
list is very long.
3) If we can use `StreamExecutionEnvironment.getExecutionEnvironment()` for
initialization instead of constuctor, we may not need the MiniCluster
configuration as the default configuration.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -453,22 +457,34 @@ private void callSet(SqlCommandCall cmdCall) {
terminal.writer()
.println(CliStrings.messageInfo(CliStrings.MESSAGE_EMPTY).toAnsi());
} else {
- properties.entrySet().stream()
- .map((e) -> e.getKey() + "=" + e.getValue())
- .sorted()
- .forEach((p) -> terminal.writer().println(p));
+ List<String> prettyEntries =
YamlConfigUtils.getPropertiesInPretty(properties);
+ prettyEntries.forEach(entry ->
terminal.writer().println(entry));
}
}
// set a property
else {
+ String key = cmdCall.operands[0].trim();
+ String value = cmdCall.operands[1].trim();
try {
- executor.setSessionProperty(
- sessionId, cmdCall.operands[0],
cmdCall.operands[1].trim());
+ executor.setSessionProperty(sessionId, key, value);
} catch (SqlExecutionException e) {
printExecutionException(e);
return;
}
-
terminal.writer().println(CliStrings.messageInfo(CliStrings.MESSAGE_SET).toAnsi());
+ if (YamlConfigUtils.isRemovedKey(key)) {
+
terminal.writer().println(CliStrings.messageWarning(MESSAGE_SET_REMOVED_KEY));
+ return;
+ } else if (YamlConfigUtils.isDeprecatedKey(key)) {
+ terminal.writer()
+ .println(
+ CliStrings.messageWarning(
+ String.format(
+ MESSAGE_SET_DEPRECATED_KEY,
+ key,
+
YamlConfigUtils.getOptionNameWithDeprecatedKey(
+ key))));
Review comment:
Should `.toAnsi`?
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -453,22 +457,34 @@ private void callSet(SqlCommandCall cmdCall) {
terminal.writer()
.println(CliStrings.messageInfo(CliStrings.MESSAGE_EMPTY).toAnsi());
} else {
- properties.entrySet().stream()
- .map((e) -> e.getKey() + "=" + e.getValue())
- .sorted()
- .forEach((p) -> terminal.writer().println(p));
+ List<String> prettyEntries =
YamlConfigUtils.getPropertiesInPretty(properties);
+ prettyEntries.forEach(entry ->
terminal.writer().println(entry));
}
}
// set a property
else {
+ String key = cmdCall.operands[0].trim();
+ String value = cmdCall.operands[1].trim();
try {
- executor.setSessionProperty(
- sessionId, cmdCall.operands[0],
cmdCall.operands[1].trim());
+ executor.setSessionProperty(sessionId, key, value);
} catch (SqlExecutionException e) {
printExecutionException(e);
return;
}
-
terminal.writer().println(CliStrings.messageInfo(CliStrings.MESSAGE_SET).toAnsi());
+ if (YamlConfigUtils.isRemovedKey(key)) {
+
terminal.writer().println(CliStrings.messageWarning(MESSAGE_SET_REMOVED_KEY));
Review comment:
Should `.toAnsi`?
----------------------------------------------------------------
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]
