-
Notifications
You must be signed in to change notification settings - Fork 616
BigQuery: Add support for BEGIN
#1718
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
Conversation
46f28fa
to
f3c803d
Compare
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.
LGTM @iffyio. I recently added support for something similar in MS-SQL, but only for the case where there's a semi-colon after the BEGIN/END TRY/CATCH keywords. I think your approach of Vec<Statement>
is better, and I'll align to that.
f3c803d
to
f018574
Compare
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.
Thanks @iffyio and @yoavcloud -- looks like a nice improvement to me!
src/ast/mod.rs
Outdated
/// END; | ||
/// ``` | ||
/// <https://cloud.google.com/bigquery/docs/reference/standard-sql/procedural-language#beginexceptionend> | ||
has_exception_when_clause: bool, |
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 wondered if this field redundant with when exception_statements
is empty? However, given the test cases below, it seems it is possible to have a EXCEPTION WHEN ERROR THEN END
(without any statements) so distinguishing is necessary
BTW another more Rusty way might be to represent this as
exception_statements: Option<Vec<Statement>>,
I don't feel strongly about this however
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.
Yeah I think that makes sense, I initially figured to use an explicit flag in case it was possible to have exception statements with a different syntax but probably no need to do that unless necessary, will update to use an option!
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 think using the somewhat akward Option<Vec<..>>
would allow the different syntax.
I can help if that would be good
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.
Done! updated to use an Option
Adds support for the `BEGIN ... EXCEPTION ... END` syntax in BigQuery: ```sql BEGIN SELECT 1; SELECT 2; EXCEPTION WHEN ERROR THEN SELECT 3; SELECT 4; END ``` https://cloud.google.com/bigquery/docs/reference/standard-sql/procedural-language#beginexceptionend
f018574
to
798f070
Compare
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.
Looks very nice @iffyio
// since display_separated doesn't handle that case. | ||
write!(f, ";")?; | ||
} | ||
if let Some(exception_statements) = exception_statements { |
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.
❤️
Adds support for the
BEGIN ... EXCEPTION ... END
syntax in BigQuery:https://cloud.google.com/bigquery/docs/reference/standard-sql/procedural-language#beginexceptionend