fsk119 commented on a change in pull request #15585: URL: https://github.com/apache/flink/pull/15585#discussion_r615272815
########## File path: flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/exception/SqlParseException.java ########## @@ -0,0 +1,32 @@ +/* + * 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.exception; + +/** Exception thrown during the parse of SQL statements. */ +public class SqlParseException extends SqlClientException { + private static final long serialVersionUID = 1L; + + public SqlParseException(String message) { + super(message); + } Review comment: This is method is not used. Remove this. ########## File path: flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java ########## @@ -320,8 +324,8 @@ private boolean executeStatement(String statement, ExecutionMode executionMode) try { final Optional<Operation> operation = parseCommand(statement); operation.ifPresent(op -> callOperation(op, executionMode)); - } catch (SqlExecutionException e) { - printExecutionException(e); + } catch (SqlClientException e) { + printSqlClientException(e); return false; Review comment: What about ``` catch (SqlParseException e) { printSqlParseException(e); } catch (SqlExecutionException e) { printSqlExecutionException(e); } ``` We can remove the `instance of`. ########## File path: flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/local/LocalExecutor.java ########## @@ -175,10 +176,10 @@ public Operation parseStatement(String sessionId, String statement) try { operations = context.wrapClassLoader(() -> parser.parse(statement)); } catch (Exception e) { - throw new SqlExecutionException("Failed to parse statement: " + statement, e); + throw new SqlParseException("Failed to parse statement: " + statement, e); } if (operations.isEmpty()) { - throw new SqlExecutionException("Failed to parse statement: " + statement); + throw new SqlExecutionException("invalid statement: " + statement); Review comment: Use `SqlParseException`? I think the origin exception message is good enough. ########## File path: flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/Executor.java ########## @@ -69,74 +70,71 @@ * <p>Both this method and {@link #getSessionConfigMap(String)} return the same configuration * set, but different return type. */ - ReadableConfig getSessionConfig(String sessionId) throws SqlExecutionException; + ReadableConfig getSessionConfig(String sessionId) throws SqlClientException; /** * Reset all the properties for the given session identifier. * * @param sessionId to identifier the session - * @throws SqlExecutionException if any error happen. + * @throws SqlClientException if any error happen. */ - void resetSessionProperties(String sessionId) throws SqlExecutionException; + void resetSessionProperties(String sessionId) throws SqlClientException; /** * Reset given key's the session property for default value, if key is not defined in config * file, then remove it. * * @param sessionId to identifier the session * @param key of need to reset the session property - * @throws SqlExecutionException if any error happen. + * @throws SqlClientException if any error happen. */ - void resetSessionProperty(String sessionId, String key) throws SqlExecutionException; + void resetSessionProperty(String sessionId, String key) throws SqlClientException; /** * Set given key's session property to the specific value. * * @param key of the session property * @param value of the session property - * @throws SqlExecutionException if any error happen. + * @throws SqlClientException if any error happen. */ - void setSessionProperty(String sessionId, String key, String value) - throws SqlExecutionException; + void setSessionProperty(String sessionId, String key, String value) throws SqlClientException; /** Parse a SQL statement to {@link Operation}. */ - Operation parseStatement(String sessionId, String statement) throws SqlExecutionException; + Operation parseStatement(String sessionId, String statement) throws SqlClientException; Review comment: nit: I think we can make the exception more specific. Use `SqlClientParseException` is better? ########## File path: flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/Executor.java ########## @@ -34,24 +35,24 @@ public interface Executor { /** Starts the executor and ensures that its is ready for commands to be executed. */ - void start() throws SqlExecutionException; + void start() throws SqlClientException; Review comment: nit: I think we can still use SqlExecutionException. we can reduce the modifcation... and it only throws SqlExecutionException here? -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
