slinkydeveloper commented on a change in pull request #18363:
URL: https://github.com/apache/flink/pull/18363#discussion_r805800991
##########
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:
I would rather implement this differently: More than having an operation
here that wraps the parsing exception etc, which then is used to properly
handle EOF by the client, I would instead have a new method in the parser that
helps with this "interactive parsing" mode.
For example, it could be some method like `interactiveParse` which returns a
union type: either the operation or the parse failure. No need to use the
operation stack for that, as it's really counterintuitive. This new method will
also allow you to get rid of this internal config option and simplify the
implementation overrall.
Also, the check based on string seems a bit fragile. Can we have a more
robust check of some kind? Is there some sub exception that calcite uses?
##########
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:
Why this change?
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/parse/CalciteParser.java
##########
@@ -57,6 +58,15 @@ public SqlNode parse(String sql) {
}
}
+ public SqlNodeList parseSqlList(String sql) {
Review comment:
Add some javadoc and also add a `throws` to specify you throw the
flink's exception, rather than the calcite one
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/parse/SetOperationParseStrategyTest.java
##########
@@ -35,6 +35,13 @@ public void testMatches() {
assertTrue(
SetOperationParseStrategy.INSTANCE.match(
"SET table.local-time-zone = 'Europe/Berlin'"));
+ assertTrue(SetOperationParseStrategy.INSTANCE.match("SET;"));
Review comment:
Use assertj
--
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]
