-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up syntax error reporting #3278
base: main
Are you sure you want to change the base?
Clean up syntax error reporting #3278
Conversation
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
31649f1
to
a47be17
Compare
{ | ||
"query" : "SELECT * FROM sample:data" | ||
"query" : "SOURCE = test_index | where a > 0)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This SQL error no longer outputs the same error message (new parsing engine?). I couldn't hit the ANTLR exception with a new SQL query, so I updated it to a PPL one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the improvement of this PR is truncate the long error message? If so, is it possible to simplify the changes especially line 60-80?
common/src/main/java/org/opensearch/sql/common/antlr/SyntaxAnalysisErrorListener.java
Show resolved
Hide resolved
common/src/main/java/org/opensearch/sql/common/antlr/SyntaxAnalysisErrorListener.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Simeon Widdis <[email protected]>
Should be better now, refactored it to only do the mapping for the first few tokens so we can just use |
common/src/main/java/org/opensearch/sql/common/antlr/SyntaxAnalysisErrorListener.java
Outdated
Show resolved
Hide resolved
common/src/main/java/org/opensearch/sql/common/antlr/SyntaxAnalysisErrorListener.java
Outdated
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/ppl/QueryAnalysisIT.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Description
While working on fixing PPL AST bugs (#3273), it stood out to me that our error reporting for syntax errors really isn't that good. This PR cleans up the handling of our errors.
Example query that's currently returning syntax errors:
Before:
After:
Even though this particular query should be valid, I think it's much clearer from this what the parser is mad about, and easier to put in a bug report too.
Related Issues
N/A
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.