slinkydeveloper commented on a change in pull request #18363:
URL: https://github.com/apache/flink/pull/18363#discussion_r807633757
##########
File path:
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/delegation/Parser.java
##########
@@ -44,7 +45,7 @@
*
* @param statement the SQL statement to evaluate
* @return parsed queries as trees of relational {@link Operation}s
- * @throws org.apache.flink.table.api.SqlParserException when failed to
parse the statement
+ * @throws SqlParserException when failed to parse the statement
Review comment:
A throws in the javadoc should be declared in the signature of the
method as well
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/SqlCommandParser.java
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.client.cli;
+
+import org.apache.flink.table.client.gateway.SqlExecutionException;
+import org.apache.flink.table.operations.Operation;
+
+import java.util.Optional;
+
+/** SqlClient command parser. */
+interface SqlCommandParser {
Review comment:
Here and in other places, make sure to use the flink annotations
`@Internal`
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -264,25 +270,43 @@ public boolean executeInitialization(String content) {
* terminal.
*/
private void executeInteractive() {
- isRunning = true;
- LineReader lineReader = createLineReader(terminal);
-
// make space from previous output and test the writer
terminal.writer().println();
terminal.writer().flush();
// print welcome
terminal.writer().append(CliStrings.MESSAGE_WELCOME);
+ LineReader lineReader = createLineReader(terminal);
+ getAndExecuteStatements(lineReader,
ExecutionMode.INTERACTIVE_EXECUTION);
+ }
+
+ private boolean getAndExecuteStatements(LineReader lineReader,
ExecutionMode mode) {
// begin reading loop
+ boolean exitOnFailure =
!mode.equals(ExecutionMode.INTERACTIVE_EXECUTION);
+ isRunning = true;
while (isRunning) {
// make some space to previous command
terminal.writer().append("\n");
terminal.flush();
- String line;
+ Optional<Operation> parsedOperation = Optional.empty();
try {
- line = lineReader.readLine(prompt, null, inputTransformer,
null);
+ String line = lineReader.readLine(prompt, null,
inputTransformer, null);
+ if (line.trim().isEmpty()) {
+ continue;
+ }
+ parsedOperation = parser.getParsedOperation();
Review comment:
I had a bit of hard time to understand how state propagation works here.
I think we can do better and make it more clear: You can
make`SqlMultiLineParser` stateless, and propagate every parsing result wrapping
`parseException` and `parsedOperation` within `SqlArgumentList` (perhaps also
rename this class), and then here you can access the last parsed
`SqlArgumentList` using `lineReader.getParsedLine()`.
With this you don't need to check any correctness of state propagation (with
the below checkArgument) and you don't need to keep the latest command line.
And it makes the code overrall simpler to understand.
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/parse/CalciteParser.java
##########
@@ -47,12 +49,36 @@ public CalciteParser(SqlParser.Config config) {
* @param sql a sql string to parse
* @return a parsed sql node
* @throws SqlParserException if an exception is thrown when parsing the
statement
+ * @throws SqlParserEOFException if the statement is incomplete
*/
public SqlNode parse(String sql) {
try {
SqlParser parser = SqlParser.create(sql, config);
return parser.parseStmt();
} catch (SqlParseException e) {
+ if (e.getMessage().contains("Encountered \"<EOF>\"")) {
+ throw new SqlParserEOFException(e.getMessage(), e);
+ }
+ throw new SqlParserException("SQL parse failed. " +
e.getMessage(), e);
+ }
+ }
+
+ /**
+ * Parses a SQL string into a {@link SqlNodeList}. The {@link SqlNodeList}
is not yet validated.
+ *
+ * @param sql a sql string to parse
+ * @return a parsed sql node list
+ * @throws SqlParserException if an exception is thrown when parsing the
statement
+ * @throws SqlParserEOFException if the statement is incomplete
+ */
+ public SqlNodeList parseSqlList(String sql) {
+ try {
+ SqlParser parser = SqlParser.create(sql, config);
+ return parser.parseStmtList();
+ } catch (SqlParseException e) {
+ if (e.getMessage().contains("Encountered \"<EOF>\"")) {
+ throw new SqlParserEOFException(e.getMessage(), e);
Review comment:
Perhaps rather than adding another exception to the stack, wouldn't it
be better to use the error kind pattern here? Like this:
https://christoffer.soop.ch/java-enumerated-fault-exceptions/
SqlParserException.Kind could have two variants now: `EOF` and `OTHER`. By
default `OTHER` is used, but you can add another constructor which takes the
kind as parameter, and then on the sql client side you use
`SqlParserException#getKind` to check if it's an EOF or not.
You should also wrap this logic of creating the Flink's `SqlParserException`
starting from calcite's `SqlParseException` in a static method in this class,
in order to make it visible.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
