slinkydeveloper commented on a change in pull request #18874:
URL: https://github.com/apache/flink/pull/18874#discussion_r811993469
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/parse/ClearOperationParseStrategy.java
##########
@@ -29,7 +29,7 @@
static final ClearOperationParseStrategy INSTANCE = new
ClearOperationParseStrategy();
private ClearOperationParseStrategy() {
- super(Pattern.compile("CLEAR;?", DEFAULT_PATTERN_FLAGS));
+ super(Pattern.compile("CLEAR(\\s*;)?", DEFAULT_PATTERN_FLAGS));
Review comment:
What about just using `CLEAR\s*;?`. This way you check any eventual
spaces, but not a space after the `;` (which should be the beginning of another
command)
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlToOperationConverterTest.java
##########
@@ -1660,6 +1665,65 @@ public void testReset() {
assertThat(((ResetOperation)
operation2).getKey()).hasValue("test-key");
}
+ @Test
+ public void testHelpCommands() {
+ ExtendedParser extendedParser = new ExtendedParser();
+ Stream.of("HELP", "HELP;", "HELP ;", "HELP\t;", "HELP\n;")
+ .forEach(
+ command -> {
+ Operation operation1 =
+ extendedParser
+ .parse(command)
+ .orElseThrow(
+ () ->
+ new
RuntimeException(
+ "Fail to
parse '"
+ +
command
+ +
"'"));
+
assertThat(operation1).isInstanceOf(HelpOperation.class);
+ });
+ }
+
+ @Test
+ public void testClearCommands() {
+ ExtendedParser extendedParser = new ExtendedParser();
+ Stream.of("CLEAR", "CLEAR;", "CLEAR ;", "CLEAR\t;", "CLEAR\n;")
+ .forEach(
+ command -> {
+ Operation operation1 =
+ extendedParser
+ .parse(command)
+ .orElseThrow(
+ () ->
+ new
RuntimeException(
+ "Fail to
parse '"
+ +
command
+ +
"'"));
+
assertThat(operation1).isInstanceOf(ClearOperation.class);
+ });
+ }
+
+ @Test
+ public void testQuitCommands() {
+ ExtendedParser extendedParser = new ExtendedParser();
+ Stream.of(
Review comment:
Please create a parametrized test for this. It's quite easy in JUnit 5,
just use `@ValueSource`:
https://junit.org/junit5/docs/current/user-guide/#writing-tests-parameterized-tests-sources-ValueSource
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlToOperationConverterTest.java
##########
@@ -1660,6 +1665,65 @@ public void testReset() {
assertThat(((ResetOperation)
operation2).getKey()).hasValue("test-key");
}
+ @Test
+ public void testHelpCommands() {
+ ExtendedParser extendedParser = new ExtendedParser();
+ Stream.of("HELP", "HELP;", "HELP ;", "HELP\t;", "HELP\n;")
+ .forEach(
+ command -> {
+ Operation operation1 =
+ extendedParser
+ .parse(command)
+ .orElseThrow(
+ () ->
+ new
RuntimeException(
+ "Fail to
parse '"
+ +
command
+ +
"'"));
+
assertThat(operation1).isInstanceOf(HelpOperation.class);
+ });
+ }
+
+ @Test
+ public void testClearCommands() {
+ ExtendedParser extendedParser = new ExtendedParser();
+ Stream.of("CLEAR", "CLEAR;", "CLEAR ;", "CLEAR\t;", "CLEAR\n;")
+ .forEach(
+ command -> {
+ Operation operation1 =
+ extendedParser
+ .parse(command)
+ .orElseThrow(
+ () ->
+ new
RuntimeException(
+ "Fail to
parse '"
+ +
command
+ +
"'"));
+
assertThat(operation1).isInstanceOf(ClearOperation.class);
+ });
+ }
+
+ @Test
+ public void testQuitCommands() {
+ ExtendedParser extendedParser = new ExtendedParser();
+ Stream.of(
+ "QUIT", "QUIT;", "QUIT ;", "QUIT\t;", "QUIT\n;",
"EXIT", "EXIT;", "EXIT ;",
+ "EXIT\t;", "EXIT\n;")
+ .forEach(
+ command -> {
+ Operation operation1 =
+ extendedParser
+ .parse(command)
+ .orElseThrow(
Review comment:
You can remove the `orElseThrow` with a proper assertj assertion:
```
assertThat(extendedParser.parse(command))
.get() // Returns an assertion on the value, and throws proper
AssertionException if no value is present
.isInstanceOf(QuitOperation.class);
```
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/parse/ClearOperationParseStrategy.java
##########
@@ -29,7 +29,7 @@
static final ClearOperationParseStrategy INSTANCE = new
ClearOperationParseStrategy();
private ClearOperationParseStrategy() {
- super(Pattern.compile("CLEAR;?", DEFAULT_PATTERN_FLAGS));
+ super(Pattern.compile("CLEAR(\\s*;)?", DEFAULT_PATTERN_FLAGS));
Review comment:
Same comments for all the ones below
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
