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. Like:
```
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?
Or,
Do we just add an overloading constructor and preserve the original
constructor?
--
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]
