-
Notifications
You must be signed in to change notification settings - Fork 616
Extend exception handling #1884
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
base: main
Are you sure you want to change the base?
Conversation
- Exception handling can handle multiple `WHEN` arms - Exception can re-raise with `RAISE` keyword - Snowflake can now also parse `BEGIN ... EXCEPTION ... END` Example: ```sql BEGIN SELECT 1; EXCEPTION WHEN EXCEPTION_1 THEN SELECT 1; WHEN EXCEPTION_2 OR EXCEPTION_3 THEN SELECT 2; SELECT 3; WHEN OTHER THEN SELECT 4; RAISE; END; ```
6f5fe30
to
6126320
Compare
src/ast/mod.rs
Outdated
/// EXCEPTION | ||
/// WHEN EXCEPTION_1 THEN | ||
/// SELECT 2; | ||
/// WHEN EXCEPTION_2 OR EXCEPTION_3 THEN | ||
/// SELECT 3; | ||
/// WHEN OTHER THEN | ||
/// SELECT 4; | ||
/// RAISE; |
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.
given the syntax example, Im not sure I see the requirement for the introduced Exception
struct - here the RAISE
looks like a normal statement that belongs either to the EXCEPTION
clause, or the BEGIN
statement itself? If the former, could it be represented as another statement in the Vec<Statement>
?, if the latter seems like its out of place in the new Exception
struct?
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.
An exception is represented by an ExceptionClause
which consists of a Vec
of ExceptionWhen
(each WHEN
arm and its statements) and a potential RAISE
statement.
For BigQuery, the docs says this which I interpreted as it can't be used without EXCEPTION
:
When USING MESSAGE isn't supplied
The RAISE statement must only be used within an EXCEPTION clause. The RAISE statement will re-raise the exception that was caught, and preserve the original stack trace.
For Snowflake we could likely do this, but there are other concerns. I know the parser isn't requiring valid syntax, but since this specific RAISE
is within error handling where you can omit the exception and re-raise what's being handled, I think it makes more sense to put it here. I would thing that if the RAISE
was on the StartTransaction
we would always have an exception as described in the Snowflake docs. It's mentioned here.
If I move the RAISE
to the StartTransaction
, should I allow it both with and without re-raising and should I move the Vec<ExceptionWhen>
directly under StartTransaction
as well?
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.
looking at snowflake and bigquery docs there doest seem to be a need for treat raise specially from what i can tell.
snowflake: their example here shows raise as a regular statement in the statement list.
bigquery: their spec also only mentions a statement list.
Also looking at this example test case, its not clear to me where the RAISE
statement applies to, and formatting wise looks different from whats in the snowflake doc like in the example above
I dont think it matters too much in which context the raise statement applies or the exact behavior of the statement in those scenarios, thats more towards semantics.
So that it seems to me that we should be able to leave as is in the previous behavior of treating RAISE
as a regular statement. Was there an example sql that wasn't covered by that approach?
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 what's your conclusion around the quote I pasted from the BigQuery docs
The RAISE statement must only be used within an EXCEPTION clause. The RAISE statement will re-raise the exception that was caught, and preserve the original stack trace.
We just treat that as semantics and put the raise directly on the StartTransaction
struct together with a Vec<ExceptionWhen>
?
So that it seems to me that we should be able to leave as is in the previous behavior of treating RAISE as a regular statement.
You mean the RAISE
should be a regular statement in the last ExceptionWhen
?
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.
I went ahead and dropped the new ExceptionClasue
which then also includes the custom handling of RAISE
. RAISE
will now be a regular statement in the block which I actually THINK is correct given something like this:
BEGIN
SELECT 1;
EXCEPTION
WHEN EXCEPTION_1 THEN
SELECT 1;
RAISE SOME_OTHER_EX;
RAISE;
END;
With this I moved all the ExceptionWhen
directly under the StartTransaction
similar to previous statement list.
src/ast/mod.rs
Outdated
/// EXCEPTION | ||
/// WHEN EXCEPTION_1 THEN | ||
/// SELECT 2; | ||
/// WHEN EXCEPTION_2 OR EXCEPTION_3 THEN | ||
/// SELECT 3; | ||
/// WHEN OTHER THEN | ||
/// SELECT 4; | ||
/// RAISE; |
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.
looking at snowflake and bigquery docs there doest seem to be a need for treat raise specially from what i can tell.
snowflake: their example here shows raise as a regular statement in the statement list.
bigquery: their spec also only mentions a statement list.
Also looking at this example test case, its not clear to me where the RAISE
statement applies to, and formatting wise looks different from whats in the snowflake doc like in the example above
I dont think it matters too much in which context the raise statement applies or the exact behavior of the statement in those scenarios, thats more towards semantics.
So that it seems to me that we should be able to leave as is in the previous behavior of treating RAISE
as a regular statement. Was there an example sql that wasn't covered by that approach?
WHEN
armsRAISE
keywordBEGIN ... EXCEPTION ... END
Example: