Skip to content

Allow empty options for BigQuery #1657

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 16, 2025
Merged

Allow empty options for BigQuery #1657

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8242d9
Allow empty options for BigQuery
MartinSahlen Jan 15, 2025
8a4ba23
Refactor based on PR feedback
Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/parser/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7341,6 +7341,10 @@ impl<'a> Parser<'a> {
pub fn parse_options(&mut self, keyword: Keyword) -> Result<Vec<SqlOption>, ParserError> {
if self.parse_keyword(keyword) {
self.expect_token(&Token::LParen)?;
if self.peek_token() == Token::RParen {
self.next_token();
return Ok(vec![]);
}
let options = self.parse_comma_separated(Parser::parse_sql_option)?;
self.expect_token(&Token::RParen)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if self.peek_token() == Token::RParen {
self.next_token();
return Ok(vec![]);
}
let options = self.parse_comma_separated(Parser::parse_sql_option)?;
self.expect_token(&Token::RParen)?;
let options = self.parse_comma_separated0(Parser::parse_sql_option, &Token::RParen)?;

Would something like this work to simplify the condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not aware of this one, but it seems to break here:

thread 'test_bigquery_create_function' panicked at src/test_utils.rs:155:61:
CREATE OR REPLACE TEMPORARY FUNCTION project1.mydataset.myfunction(x FLOAT64) RETURNS FLOAT64 OPTIONS(x = 'y') AS 42: ParserError("Expected: AS, found: )")

when it looks like this

  pub fn parse_options(&mut self, keyword: Keyword) -> Result<Vec<SqlOption>, ParserError> {
        if self.parse_keyword(keyword) {
            self.expect_token(&Token::LParen)?;
            let options = self.parse_comma_separated0(Parser::parse_sql_option, Token::RParen)?;
            Ok(options)
        } else {
            Ok(vec![])
        }
    }

I will work a bit to see what is going wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in the function parse_comma_separated0 should be equivalent to what I did already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried changing both
parse_options and parse_options_with_keywords as they excibit similar behaviour but I cannot get the tests to pass, although my original use case is working. It could be that I've stumbled upon some deeper issue or bug though (or I dont know enough about the code base which is clearly the case). The question is whether this potential bug should be addressed in this PR or not. Based on the expected bevahour of the function you suggested this seems like a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I found the bug, if I do this:

  pub fn parse_options(&mut self, keyword: Keyword) -> Result<Vec<SqlOption>, ParserError> {
        if self.parse_keyword(keyword) {
            self.expect_token(&Token::LParen)?;
            let options = self.parse_comma_separated0(Parser::parse_sql_option, Token::RParen)?;
            self.expect_token(&Token::RParen)?;
            Ok(options)
        } else {
            Ok(vec![])
        }
    }

its is working. And this leads me to look at the parse_comma_separated0 implementation. Should this not also consume the end_token parameter?

 pub fn parse_comma_separated0<T, F>(
        &mut self,
        f: F,
        end_token: Token,
    ) -> Result<Vec<T>, ParserError>

Ok I tried making this function consume the end_token but it seems to cause more issues. I also saw that there is a parse_options_with_keywords that at the moment will not accept an empty options, but I will not change or touch that in this PR.

Ok(options)
Expand Down
12 changes: 12 additions & 0 deletions tests/sqlparser_bigquery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2244,3 +2244,15 @@ fn test_any_type() {
fn test_any_type_dont_break_custom_type() {
bigquery_and_generic().verified_stmt("CREATE TABLE foo (x ANY)");
}

#[test]
fn parse_create_table_with_empty_table_options() {
let sql = "CREATE TABLE foo (x INT64) OPTIONS()";
bigquery().verified_stmt(sql);
}

#[test]
fn parse_create_table_with_empty_table_options_and_column_options() {
let sql = "CREATE TABLE db.schema.test (x INT64 OPTIONS(description = 'An optional INTEGER field')) OPTIONS()";
bigquery().verified_stmt(sql);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add these as scenarios into the existing tests for the feature vs having them as dedicated tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shall be done! How did I miss that, I was searching but my bad!

Loading