[GitHub] [shardingsphere] lhyundeadsoul commented on a diff in pull request #20677: optimize SQLParsingException stack to show the detailed parse error

[GitBox <gi...@apache.org>]

lhyundeadsoul commented on code in PR #20677:
URL: https://github.com/apache/shardingsphere/pull/20677#discussion_r969240760


##########
shardingsphere-sql-parser/shardingsphere-sql-parser-engine/src/main/java/org/apache/shardingsphere/sql/parser/core/database/parser/SQLParserExecutor.java:
##########
@@ -64,7 +64,7 @@ private ParseASTNode twoPhaseParse(final String sql) {
             try {
                 return (ParseASTNode) sqlParser.parse();
             } catch (final ParseCancellationException e) {
-                throw new SQLParsingException(sql + ", " + e.getMessage());

Review Comment:
   We concat sql and error message in constructor parameter here, which only refers to SQL. That is not reasonable.
   
   I think it's better to separate SQL and error message for unambiguous.
   
   ```
       public SQLParsingException(final String sql, final String detail) {
           super(XOpenSQLState.SYNTAX_ERROR, KERNEL_CODE, 0, "You have an error in your SQL syntax: %s, %s", sql, detail);
       }
   ```
   But according to https://github.com/apache/shardingsphere/pull/20702#issuecomment-1234417673  each exception can only have one type of message. If I add the second parameter. All places that use this Exception have to be refactored. 
   Is that right and necessary?
   



-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org