wenlong88 commented on a change in pull request #18363:
URL: https://github.com/apache/flink/pull/18363#discussion_r806424411
##########
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));
Review comment:
it is because that in this pr, we didn't truncate ';' in sql client,
parser need to support commands in both 'clear' and 'clear;'
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/delegation/ParserImpl.java
##########
@@ -95,12 +105,24 @@ public ParserImpl(
}
// parse the sql query
- SqlNode parsed = parser.parse(statement);
-
- Operation operation =
- SqlToOperationConverter.convert(planner, catalogManager,
parsed)
- .orElseThrow(() -> new TableException("Unsupported
query: " + statement));
- return Collections.singletonList(operation);
+ try {
+ // use parseSqlList here because we need to support statement end
with ';' in sql
+ // client.
+ SqlNodeList sqlNodeList = parser.parseSqlList(statement);
+ List<SqlNode> parsed = sqlNodeList.getList();
+ Preconditions.checkArgument(parsed.size() == 1, "only single
statement supported");
+ return Collections.singletonList(
+ SqlToOperationConverter.convert(planner, catalogManager,
parsed.get(0))
+ .orElseThrow(
+ () -> new TableException("Unsupported
query: " + statement)));
+ } catch (SqlParserException parserException) {
+ if
(config.getConfiguration().getBoolean(TABLE_PARSER_ALLOW_PARTIAL_PARSE)
+ && parserException.getMessage().contains("Encountered
\"<EOF>\"")) {
+ return Collections.singletonList(new
StagingOperation(statement, parserException));
+ } else {
+ throw parserException;
+ }
Review comment:
thanks for the advise, I am also thinking about how to get rid of the
internal config based on the Godfreyhe 's comment. I like the idea adding a new
function, but I would like to keep the StagingOperation instead of a union
type, because it make it easier to transporting the result between different
components in parser/client.
I am afraid that we can only check based on string, because the calcite
parser only format the message to describe the error, there is no sub exception.
WDYT?
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/delegation/ParserImpl.java
##########
@@ -76,16 +81,42 @@ public ParserImpl(
this.sqlExprToRexConverterFactory = sqlExprToRexConverterFactory;
}
+ /**
+ * Parses a given statement to {@link Operation}. the statement should be
complete and valid, or
+ * an {@link SqlParserException} would be thrown.
+ *
+ * @param statement input statement.
+ * @return parsed operations.
+ */
+ @Override
+ public List<Operation> parse(String statement) {
+ return parseInternal(statement, false);
+ }
+
+ /**
+ * Parses a given statement interactively to {@link Operation}, used in
sql client. Different
+ * from {@link #parse(String)}, it returns a {@link StagingOperation} when
given an incomplete
Review comment:
hi, I have investigated again. currently we don't need the new function:
when the statement is incomplete, parser throws SqlParserEOFException, and we
can just check the exception in sql client.
WDYT?
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/delegation/ParserImpl.java
##########
@@ -76,16 +81,42 @@ public ParserImpl(
this.sqlExprToRexConverterFactory = sqlExprToRexConverterFactory;
}
+ /**
+ * Parses a given statement to {@link Operation}. the statement should be
complete and valid, or
+ * an {@link SqlParserException} would be thrown.
+ *
+ * @param statement input statement.
+ * @return parsed operations.
+ */
+ @Override
+ public List<Operation> parse(String statement) {
+ return parseInternal(statement, false);
+ }
+
+ /**
+ * Parses a given statement interactively to {@link Operation}, used in
sql client. Different
+ * from {@link #parse(String)}, it returns a {@link StagingOperation} when
given an incomplete
Review comment:
I have reverted the new added interactiveParse and StagingOperation.
--
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]
