You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2022/06/25 19:49:23 UTC
[GitHub] [shardingsphere] ninjaboy97 opened a new pull request, #18598: Print error SQL when throwing exception.SQLParsingException
ninjaboy97 opened a new pull request, #18598:
URL: https://github.com/apache/shardingsphere/pull/18598
Fixes #18455 .
Changes proposed in this pull request:
- Throw root cause of SQLParsingException
--
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
[GitHub] [shardingsphere] terrymanu commented on pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
terrymanu commented on PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#issuecomment-1189781109
@ninjaboy97 Hi, any update?
--
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
[GitHub] [shardingsphere] terrymanu closed pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
terrymanu closed pull request #18598: Print error SQL when throwing exception.SQLParsingException
URL: https://github.com/apache/shardingsphere/pull/18598
--
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
[GitHub] [shardingsphere] terrymanu commented on pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
terrymanu commented on PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#issuecomment-1212708925
I just close this PR and reopen issue #18455.
--
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
[GitHub] [shardingsphere] terrymanu commented on a diff in pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
terrymanu commented on code in PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#discussion_r906759441
##########
shardingsphere-distsql/shardingsphere-distsql-parser/src/main/java/org/apache/shardingsphere/distsql/parser/core/advanced/AdvancedDistSQLStatementParserEngine.java:
##########
@@ -40,12 +40,14 @@ public SQLStatement parse(final String sql) {
ASTNode astNode = parseToASTNode(sql);
return getSQLStatement(sql, (ParseASTNode) astNode);
}
-
+
private ASTNode parseToASTNode(final String sql) {
try {
return SQLParserFactory.newInstance(sql, AdvancedDistSQLLexer.class, AdvancedDistSQLParser.class).parse();
- } catch (final ParseCancellationException | SQLParsingException ignored) {
- throw new SQLParsingException("You have an error in your SQL syntax.");
+ } catch (final ParseCancellationException ignored) {
+ throw new SQLParsingException("You have an error in your SQL syntax: " + sql);
+ } catch (final SQLParsingException sqlParsingException) {
+ throw sqlParsingException;
Review Comment:
Can you try to input an error SQL from JDBC and Proxy, and parse the exception here.
I want to make sure the exception thrown on the right way.
--
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
[GitHub] [shardingsphere] ninjaboy97 commented on a diff in pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
ninjaboy97 commented on code in PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#discussion_r906753107
##########
shardingsphere-distsql/shardingsphere-distsql-parser/src/main/java/org/apache/shardingsphere/distsql/parser/core/advanced/AdvancedDistSQLStatementParserEngine.java:
##########
@@ -40,12 +40,14 @@ public SQLStatement parse(final String sql) {
ASTNode astNode = parseToASTNode(sql);
return getSQLStatement(sql, (ParseASTNode) astNode);
}
-
+
private ASTNode parseToASTNode(final String sql) {
try {
return SQLParserFactory.newInstance(sql, AdvancedDistSQLLexer.class, AdvancedDistSQLParser.class).parse();
- } catch (final ParseCancellationException | SQLParsingException ignored) {
- throw new SQLParsingException("You have an error in your SQL syntax.");
+ } catch (final ParseCancellationException ignored) {
+ throw new SQLParsingException("You have an error in your SQL syntax: " + sql);
+ } catch (final SQLParsingException sqlParsingException) {
+ throw sqlParsingException;
Review Comment:
I couldn't test this line from the tests. Any invalid sql I generate throws ParseCancellationException and therefore line 48 is being tested. Any advice on how I can test this line @terrymanu?
--
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
[GitHub] [shardingsphere] ninjaboy97 commented on a diff in pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
ninjaboy97 commented on code in PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#discussion_r906753000
##########
shardingsphere-distsql/shardingsphere-distsql-parser/src/main/java/org/apache/shardingsphere/distsql/parser/core/advanced/AdvancedDistSQLStatementParserEngine.java:
##########
@@ -40,12 +40,14 @@ public SQLStatement parse(final String sql) {
ASTNode astNode = parseToASTNode(sql);
return getSQLStatement(sql, (ParseASTNode) astNode);
}
-
+
private ASTNode parseToASTNode(final String sql) {
try {
return SQLParserFactory.newInstance(sql, AdvancedDistSQLLexer.class, AdvancedDistSQLParser.class).parse();
- } catch (final ParseCancellationException | SQLParsingException ignored) {
- throw new SQLParsingException("You have an error in your SQL syntax.");
+ } catch (final ParseCancellationException ignored) {
+ throw new SQLParsingException("You have an error in your SQL syntax: " + sql);
+ } catch (final SQLParsingException sqlParsingException) {
+ throw sqlParsingException;
Review Comment:
The ParseCancellationException does not include SQL and parse error cause.
--
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
[GitHub] [shardingsphere] ninjaboy97 commented on pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
ninjaboy97 commented on PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#issuecomment-1174734797
Hey! I'll be able to update this by the coming weekend.
--
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
[GitHub] [shardingsphere] terrymanu commented on pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
terrymanu commented on PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#issuecomment-1167494219
I'am expecting the screenshot for exception thrown when input type SQL
--
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
[GitHub] [shardingsphere] codecov-commenter commented on pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#issuecomment-1166415307
# [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/18598?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18598](https://codecov.io/gh/apache/shardingsphere/pull/18598?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (590f1c9) into [master](https://codecov.io/gh/apache/shardingsphere/commit/ed72b893588664ea3eef1a978a4c9cc23dab5bc8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ed72b89) will **decrease** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #18598 +/- ##
============================================
- Coverage 59.32% 59.31% -0.01%
Complexity 2255 2255
============================================
Files 3717 3717
Lines 54724 54726 +2
Branches 9326 9326
============================================
Hits 32463 32463
- Misses 19510 19512 +2
Partials 2751 2751
```
| [Impacted Files](https://codecov.io/gh/apache/shardingsphere/pull/18598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...advanced/AdvancedDistSQLStatementParserEngine.java](https://codecov.io/gh/apache/shardingsphere/pull/18598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZGlzdHNxbC9zaGFyZGluZ3NwaGVyZS1kaXN0c3FsLXBhcnNlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvZGlzdHNxbC9wYXJzZXIvY29yZS9hZHZhbmNlZC9BZHZhbmNlZERpc3RTUUxTdGF0ZW1lbnRQYXJzZXJFbmdpbmUuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...featured/FeaturedDistSQLStatementParserEngine.java](https://codecov.io/gh/apache/shardingsphere/pull/18598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZGlzdHNxbC9zaGFyZGluZ3NwaGVyZS1kaXN0c3FsLXBhcnNlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvZGlzdHNxbC9wYXJzZXIvY29yZS9mZWF0dXJlZC9GZWF0dXJlZERpc3RTUUxTdGF0ZW1lbnRQYXJzZXJFbmdpbmUuamF2YQ==) | `0.00% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/shardingsphere/pull/18598?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/18598?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ed72b89...590f1c9](https://codecov.io/gh/apache/shardingsphere/pull/18598?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [shardingsphere] terrymanu commented on a diff in pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
terrymanu commented on code in PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#discussion_r906759441
##########
shardingsphere-distsql/shardingsphere-distsql-parser/src/main/java/org/apache/shardingsphere/distsql/parser/core/advanced/AdvancedDistSQLStatementParserEngine.java:
##########
@@ -40,12 +40,14 @@ public SQLStatement parse(final String sql) {
ASTNode astNode = parseToASTNode(sql);
return getSQLStatement(sql, (ParseASTNode) astNode);
}
-
+
private ASTNode parseToASTNode(final String sql) {
try {
return SQLParserFactory.newInstance(sql, AdvancedDistSQLLexer.class, AdvancedDistSQLParser.class).parse();
- } catch (final ParseCancellationException | SQLParsingException ignored) {
- throw new SQLParsingException("You have an error in your SQL syntax.");
+ } catch (final ParseCancellationException ignored) {
+ throw new SQLParsingException("You have an error in your SQL syntax: " + sql);
+ } catch (final SQLParsingException sqlParsingException) {
+ throw sqlParsingException;
Review Comment:
Can you try to input an error SQL from JDBC and Proxy, and parse the exception here.
I want to make sure the exception thrown on the right way.
For JDBC, exception message is enough
For Proxy, provide a screenshot by MySQL cli is better
--
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
[GitHub] [shardingsphere] ninjaboy97 commented on a diff in pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
ninjaboy97 commented on code in PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#discussion_r906753107
##########
shardingsphere-distsql/shardingsphere-distsql-parser/src/main/java/org/apache/shardingsphere/distsql/parser/core/advanced/AdvancedDistSQLStatementParserEngine.java:
##########
@@ -40,12 +40,14 @@ public SQLStatement parse(final String sql) {
ASTNode astNode = parseToASTNode(sql);
return getSQLStatement(sql, (ParseASTNode) astNode);
}
-
+
private ASTNode parseToASTNode(final String sql) {
try {
return SQLParserFactory.newInstance(sql, AdvancedDistSQLLexer.class, AdvancedDistSQLParser.class).parse();
- } catch (final ParseCancellationException | SQLParsingException ignored) {
- throw new SQLParsingException("You have an error in your SQL syntax.");
+ } catch (final ParseCancellationException ignored) {
+ throw new SQLParsingException("You have an error in your SQL syntax: " + sql);
+ } catch (final SQLParsingException sqlParsingException) {
+ throw sqlParsingException;
Review Comment:
I couldn't test line 50 from the tests. Any invalid sql I try throws ParseCancellationException and therefore line 48 is being tested. Any advice on how I can test this line @terrymanu?
--
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
[GitHub] [shardingsphere] terrymanu commented on a diff in pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
terrymanu commented on code in PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#discussion_r906759341
##########
shardingsphere-distsql/shardingsphere-distsql-parser/src/main/java/org/apache/shardingsphere/distsql/parser/core/advanced/AdvancedDistSQLStatementParserEngine.java:
##########
@@ -40,12 +40,14 @@ public SQLStatement parse(final String sql) {
ASTNode astNode = parseToASTNode(sql);
return getSQLStatement(sql, (ParseASTNode) astNode);
}
-
+
private ASTNode parseToASTNode(final String sql) {
try {
return SQLParserFactory.newInstance(sql, AdvancedDistSQLLexer.class, AdvancedDistSQLParser.class).parse();
- } catch (final ParseCancellationException | SQLParsingException ignored) {
- throw new SQLParsingException("You have an error in your SQL syntax.");
+ } catch (final ParseCancellationException ignored) {
+ throw new SQLParsingException("You have an error in your SQL syntax: " + sql);
+ } catch (final SQLParsingException sqlParsingException) {
+ throw sqlParsingException;
Review Comment:
ParseCancellationException maybe throw from parse mode error
--
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
[GitHub] [shardingsphere] ninjaboy97 commented on a diff in pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
ninjaboy97 commented on code in PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#discussion_r906753107
##########
shardingsphere-distsql/shardingsphere-distsql-parser/src/main/java/org/apache/shardingsphere/distsql/parser/core/advanced/AdvancedDistSQLStatementParserEngine.java:
##########
@@ -40,12 +40,14 @@ public SQLStatement parse(final String sql) {
ASTNode astNode = parseToASTNode(sql);
return getSQLStatement(sql, (ParseASTNode) astNode);
}
-
+
private ASTNode parseToASTNode(final String sql) {
try {
return SQLParserFactory.newInstance(sql, AdvancedDistSQLLexer.class, AdvancedDistSQLParser.class).parse();
- } catch (final ParseCancellationException | SQLParsingException ignored) {
- throw new SQLParsingException("You have an error in your SQL syntax.");
+ } catch (final ParseCancellationException ignored) {
+ throw new SQLParsingException("You have an error in your SQL syntax: " + sql);
+ } catch (final SQLParsingException sqlParsingException) {
+ throw sqlParsingException;
Review Comment:
I couldn't test line 50 from the tests. Any invalid sql I generate throws ParseCancellationException and therefore line 48 is being tested. Any advice on how I can test this line @terrymanu?
--
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
[GitHub] [shardingsphere] terrymanu commented on a diff in pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
terrymanu commented on code in PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#discussion_r906751527
##########
shardingsphere-distsql/shardingsphere-distsql-parser/src/main/java/org/apache/shardingsphere/distsql/parser/core/advanced/AdvancedDistSQLStatementParserEngine.java:
##########
@@ -40,12 +40,14 @@ public SQLStatement parse(final String sql) {
ASTNode astNode = parseToASTNode(sql);
return getSQLStatement(sql, (ParseASTNode) astNode);
}
-
+
private ASTNode parseToASTNode(final String sql) {
try {
return SQLParserFactory.newInstance(sql, AdvancedDistSQLLexer.class, AdvancedDistSQLParser.class).parse();
- } catch (final ParseCancellationException | SQLParsingException ignored) {
- throw new SQLParsingException("You have an error in your SQL syntax.");
+ } catch (final ParseCancellationException ignored) {
+ throw new SQLParsingException("You have an error in your SQL syntax: " + sql);
+ } catch (final SQLParsingException sqlParsingException) {
+ throw sqlParsingException;
Review Comment:
Why separate this two exception, how about just throw together?
Does the ParseCancellationException include SQL and parse error cause?
--
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
[GitHub] [shardingsphere] terrymanu commented on pull request #18598: Print error SQL when throwing exception.SQLParsingException
Posted by GitBox <gi...@apache.org>.
terrymanu commented on PR #18598:
URL: https://github.com/apache/shardingsphere/pull/18598#issuecomment-1207440723
@ninjaboy97 The progress of this PR has not been upgraded for a long time. If there is no update, I will close it soon.
--
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