[FLINK-29486][sql-client] Implement ClientParser for implementing remote SQL client later #20931

yuzelin · 2022-09-30T09:55:15Z

What is the purpose of the change

The current SqlCommandParserImpl for SQL client use the paser of embedded TableEnvironment, which cannot be fetched in remote mode. So this PR introduce a new parser.

Brief change log

Adjust the package structure for parsers.
Modify SqlCommandParser for convenience:
- extends FlinkSqlParserImplConstants to use defined token kinds
- add parseStatement method.
implement ClientParser and add corresponding test.

Verifying this change

This change added test: ClientParserTest.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

flinkbot · 2022-09-30T10:03:11Z

CI report:

4a5bd6e Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

fsk119

Thanks for your contribution. I left some comments.

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

fsk119 · 2022-10-02T15:03:23Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+        if (firstToken.kind == IDENTIFIER) {
+            // unrecognized token
+            return getPotentialCommandType(firstToken.image);
+        } else if (firstToken.kind == EXPLAIN) {
+            return Optional.of(StatementType.EXPLAIN);
+        } else if (firstToken.kind == SHOW) {
+            return getPotentialShowCreateType(tokenList);
+        } else {
+            return Optional.of(StatementType.OTHER);
+        }


When the statement is not ends with ;, it is not a statement. But it seems we will always return OTHER?

To get a completed SQL statement is the function of the SqlMultiLineParser. So I think we can make sure that statement here should be ended with ';'. And now I think all the test data should be ended with ';'.

After checking the implementation of SqlMultiLineParser, here when the statement is incomplete, an SqlExecutionException should be thrown. I added the codes.

fsk119 · 2022-10-02T15:04:55Z

...e/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/StatementType.java

+package org.apache.flink.table.client.cli.parser;
+
+/** Enumerates the possible types of input statements. */
+public enum StatementType {


How about BEGIN STATEMENT SET/END?

Added these two types and corresponding tests.

fsk119 · 2022-10-02T15:17:39Z

flink-table/flink-sql-client/pom.xml

+        <dependency>
+            <groupId>org.apache.flink</groupId>
+            <artifactId>flink-sql-parser</artifactId>
+            <version>${project.version}</version>
+        </dependency>


Take a look at the shade plugin below. We can do as table-planner module append the parser class into the final jar. But I think we don't need all classes in the parser jar and we can filter out if it is not needed.

Added new configuration to shade plugin. I pushed a new commit to see if it works.

Added new configuration to shade plugin. I pushed a new commit to see if it works.

It works.

fsk119 · 2022-10-02T15:19:41Z

...link-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/SqlCommandParser.java

 @Internal
-interface SqlCommandParser {
+public interface SqlCommandParser extends FlinkSqlParserImplConstants {


Why this interface extends FlinkSqlParserImplConstants? I think ClientParser extends this is enough.

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

fsk119 · 2022-10-02T15:30:24Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+        do {
+            token = tokenManager.getNextToken();
+            tokenList.add(token);
+        } while (token.endColumn != trimmedStatement.length());


I think we can condition can be token != null

After checking the codes of getNextToken, I found that when the input stream is over, it won't return null but a EOF token. So I think use condition token.kind != EOF may be better?

fsk119 · 2022-10-02T15:31:06Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+    }
+
+    // ---------------------------------------------------------------------------------------------
+    private Optional<StatementType> getStatementType(List<Token> tokenList) {


Can we use itearor model here? We can reduce loop twice to once

I think use iterator model here won't save time, but it will make the codes more complicated. I think an ArrayList here is just OK.

fsk119 · 2022-10-02T15:33:22Z

...link-sql-client/src/test/java/org/apache/flink/table/client/cli/parser/ClientParserTest.java

+
+    private static List<Tuple2<String, Optional<StatementType>>> generateTestData() {
+        return Arrays.asList(
+                Tuple2.of("quit;", QUIT),


Use a TestSpec rather than Tuple2 here. TestSpec has better semantic. BTW, when user inputs quit, the terminal should contine reading from the input.

fsk119 · 2022-10-02T15:34:15Z

...link-sql-client/src/test/java/org/apache/flink/table/client/cli/parser/ClientParserTest.java

+                Tuple2.of("SHOW CREATE TABLE(what_ever);", SHOW_CREATE),
+                Tuple2.of("SHOW CREATE VIEW (what_ever)", SHOW_CREATE),
+                Tuple2.of("SHOW CREATE syntax_error;", OTHER),
+                Tuple2.of("--SHOW CREATE TABLE ignore_comment", EMPTY),


Test SHOW TABLES -- comment ; and muli-line cases:

SHOW\n create\t TABLE `tbl`;

Take a look at presto test cases TestStatementSplitter

Added some tests.

fsk119

Thanks for your update. I left some comments.

fsk119 · 2022-10-03T12:44:45Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+    /** A dumb implementation. TODO: remove this after unifying the SqlMultiLineParser. */
+    @Override
+    public Optional<Operation> parseCommand(String command) {
+        return Optional.empty();


I think it should be

parseStatement(statement); return Optional.empty();

I think this method won't be called, just override the interface's method.

fsk119 · 2022-10-03T12:45:51Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+        return Optional.empty();
+    }
+
+    public Optional<StatementType> parseStatement(@Nonnull String statement)


we don't need to add @Nonnull annoation. By default, we assume the parameter is not null and we only add @Nullable if it is nullable.

fsk119 · 2022-10-03T12:54:18Z

...link-sql-client/src/test/java/org/apache/flink/table/client/cli/parser/ClientParserTest.java

+    @MethodSource("generateTestData")
+    public void testParseStatement(TestSpec testData) {
+        Optional<StatementType> type = clientParser.parseStatement(testData.statement);
+        assertThat(type).isEqualTo(testData.type);


nit: I think it's better to use

assertThat(type.orElse(null)).isEqualTo(testData.type);

We can also remove SuppressWarnings above.

fsk119 · 2022-10-03T12:55:44Z

...link-sql-client/src/test/java/org/apache/flink/table/client/cli/parser/ClientParserTest.java

+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"", "\n", " ", "-- comment;", "SHOW TABLES -- comment;", "SHOW TABLES"})


Add a case EXPLAIN STATEMENT SET BEGIN INSERT INTO StreamingTable SELECT * FROM (VALUES (1, 'Hello World'), (2, 'Hi'), (2, 'Hi'), (3, 'Hello'), (3, 'World'), (4, 'ADD'), (5, 'LINE'));

fsk119 · 2022-10-03T13:26:54Z

...link-sql-client/src/test/java/org/apache/flink/table/client/cli/parser/ClientParserTest.java

+
+    private static List<TestSpec> generateTestData() {
+        return Arrays.asList(
+                TestSpec.of(";", null),


Why we stilll need null? What's the difference between OTHER and null?

The ';' is very special. if we just typed a ';' , the terminal should ignore this input, and the null return will finally become Optional.empty() and notify the terminal to ignore the return; An OTHER means the parser cannot recognize the statement's type and will submit it to cluster. And if we want to notify the terminal that the input is incomplete we throw an exception.

fsk119 · 2022-10-03T13:28:22Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+
+        if (tokens.size() == 0 || tokens.get(tokens.size() - 1).kind != SEMICOLON) {
+            // throw this to notify the terminal to continue reading input
+            throw new SqlExecutionException("", new SqlParserEOFException(""));


Add meaningful exception msg here.

fsk119 · 2022-10-03T15:00:09Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+
+/**
+ * ClientParser use {@link FlinkSqlParserImplTokenManager} to do lexical analysis. It cannot
+ * recognize special hive keywords yet.


Hive has a slightly different vocabulary compared to Flink vocabulary, which causes the ClientParser will misunderstand Hive's keywords to IDENTIFIER. But the ClientParser is only responsible to check whether the statement is completed or not and only cares about a few statements. So it's acceptable to tolerate the inaccuracy here.

Added more description.

fsk119 · 2022-10-03T15:02:09Z

...e/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/StatementType.java

+
+/** Enumerates the possible types of input statements. */
+public enum StatementType {
+    QUIT,


nit: It's better we can add detailed msg for every types...

fsk119

After rethink the design, I think the ClientParser also requires to return the trimmed sql(without semicolon) to the executor.

fsk119 · 2022-10-05T01:18:15Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+                    new FlinkSqlParserImplTokenManager(
+                            new SimpleCharStream(new StringReader(statement)));
+            // means to switch to "BACK QUOTED IDENTIFIER" state to support '`xxx`' in Flink SQL
+            tokenManager.SwitchTo(2);


add comment

please visit {@link CalciteParser}#createFlinkParser for more details.

fsk119 · 2022-10-05T01:39:52Z

...link-sql-client/src/test/java/org/apache/flink/table/client/cli/parser/ClientParserTest.java

+    }
+
+    /** Used to load generated data. */
+    private static class TestSpec {


add toString method

fsk119 · 2022-10-05T01:50:36Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+                continueReadInput();
+            }
+
+            Token head = currentToken, tail = tokenManager.getNextToken();


head && tail is a little confusing to me. I think it's better to use current and next

fsk119 · 2022-10-05T01:52:45Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+            tokenManager =
+                    new FlinkSqlParserImplTokenManager(
+                            new SimpleCharStream(new StringReader(statement)));
+            // means to switch to "BACK QUOTED IDENTIFIER" state to support '`xxx`' in Flink SQL


replace xxx to <IDENTIFIER>

fsk119 · 2022-10-05T01:55:33Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+                            : StatementType.OTHER);
+        } else if (firstToken.kind == BEGIN) {
+            return Optional.of(
+                    tokens.nextTokenMatched(STATEMENT) && tokens.nextTokenMatched(SET)


the nextTokenMatched may mislead others. It also move forward the pointer to the next token. Why we don't introduce lookAhead here like we do in the ParserImpl.ftl?

fsk119 · 2022-10-05T02:02:06Z

...le/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/parser/ClientParser.java

+            Token head = currentToken, tail = tokenManager.getNextToken();
+
+            // case 2, 3 and 4
+            boolean setNotEnded =


The explain also supports executing statement set with explain details. I think we need to rethink the behavior here.

fsk119 · 2022-10-10T11:20:29Z

The failed test is caused by the FLINK-29405

fsk119 · 2022-10-10T11:20:38Z

@flinkbot run azure

fsk119 · 2022-10-11T01:52:59Z

@flinkbot run azure

yuzelin · 2022-10-11T04:26:31Z

@flinkbot run azure

…ote SQL client later.

fsk119

Thanks for your update. LGTM.

…e mode (apache#20931)

flinkbot added the component=TableSQL/Client label Sep 30, 2022

fsk119 reviewed Oct 2, 2022

View reviewed changes

yuzelin force-pushed the ClientParser branch 2 times, most recently from 4fa645b to 933902b Compare October 3, 2022 11:18

fsk119 reviewed Oct 3, 2022

View reviewed changes

fsk119 reviewed Oct 5, 2022

View reviewed changes

yuzelin force-pushed the ClientParser branch from d872a4b to 21b5b2b Compare October 8, 2022 06:05

fsk119 force-pushed the ClientParser branch from 958c69a to 6fb39dc Compare October 9, 2022 12:51

yuzelin force-pushed the ClientParser branch from cff4684 to 6b930d1 Compare October 10, 2022 06:49

yuzelin and others added 11 commits October 11, 2022 14:10

[FLINK-29486][sql-client] Implement ClientParser for implementing rem…

ca38550

…ote SQL client later.

correct pom.xml of flink-sql-client.

0ba1415

Add new configuration to shade plugin.

5735bd8

apply review comments.

38c5708

Added code to handle special cases of EXPALIN statement.

ca96502

apply review comment.

d974697

Add SET and RESET to StatementType.

1bbdc1c

Add REMOVE_JAR to StatementType.

b81e783

[fix] improve codes

834c568

[fix] correct pom dependency.

67e8a62

[fix] minor improves.

712edd2

yuzelin force-pushed the ClientParser branch from 8b65681 to 712edd2 Compare October 11, 2022 06:11

[fix] correct pom dependency.

4a5bd6e

fsk119 approved these changes Oct 12, 2022

View reviewed changes

fsk119 merged commit 205f70c into apache:master Oct 12, 2022

yuzelin deleted the ClientParser branch October 13, 2022 01:53

huangxiaofeng10047 pushed a commit to huangxiaofeng10047/flink that referenced this pull request Nov 3, 2022

[FLINK-29486][sql-client] Introduce ClientParser for SQL Client remot…

abd97f5

…e mode (apache#20931)

[FLINK-29486][sql-client] Implement ClientParser for implementing remote SQL client later #20931

[FLINK-29486][sql-client] Implement ClientParser for implementing remote SQL client later #20931

Conversation

yuzelin commented Sep 30, 2022

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Sep 30, 2022 • edited

CI report:

fsk119 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuzelin Oct 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuzelin Oct 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsk119 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsk119 Oct 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsk119 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsk119 Oct 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsk119 commented Oct 10, 2022

fsk119 commented Oct 10, 2022

fsk119 commented Oct 11, 2022

yuzelin commented Oct 11, 2022

fsk119 left a comment

Choose a reason for hiding this comment

flinkbot commented Sep 30, 2022 •

edited

yuzelin Oct 2, 2022 •

edited

yuzelin Oct 3, 2022 •

edited

fsk119 Oct 3, 2022 •

edited

fsk119 Oct 5, 2022 •

edited