Allow empty options for BigQuery

[MartinSahlen]

In BigQuery, it's possible to have statements like these (added tests for them):

CREATE TABLE foo (x INT64) OPTIONS()

CREATE TABLE db.schema.test (x INT64 OPTIONS(description = 'An optional INTEGER field')) OPTIONS()

While this may look silly (and clearly is), it's very common that companies are using tools (like dbt) to generate the SQL based on a certain configuration defined at runtime using macros and other abominations (but that is off-topic). If there is no config/options, the clause is still generated, with no contents. I am not sure about the semantics in other platforms, but PR, with tests, allows specifying empty options. Seeing that all tests run, this can hopefully be merged quickly. Also ran fmt and clippy.

[iffyio]

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?

[MartinSahlen]

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

[MartinSahlen]

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

[MartinSahlen]

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.

[MartinSahlen]

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.

[iffyio]

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

[MartinSahlen]

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

[iffyio]

LGTM! Thanks @MartinSahlen!
cc @alamb