Skip to content
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

BigQuery partition by in create table. #31

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

zdenal
Copy link

@zdenal zdenal commented Mar 5, 2024

Why

BigQuery create table statement is supporting PARTITION BY <expr> after column difinition. This PR is allowing us to parse these statements.

BigQuery doc

@@ -4083,6 +4083,13 @@ impl<'a> Parser<'a> {
None
};

// In BigQuery PARITION BY can be also defined after columns
let partitioned_by = if self.parse_keywords(&[Keyword::PARTITION, Keyword::BY]) {
Some(self.parse_expr()?)
Copy link
Author

Choose a reason for hiding this comment

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

Went through code base and parsing PARTITION BY or PARTITION in parts like window specification are always parsing just expr. Keeping the same parsing.

Also not guarding it by dialect as whole parse_create_table is dialect independent.

let res = Parser::new(&dialect)
.try_with_sql(&sql)
.expect("tokenize to work")
.with_recursion_limit(38)
Copy link
Author

Choose a reason for hiding this comment

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

The stack frame size increased in debug builds as this PR adds new local variables.
I fixed the test by decreasing recursion limit (39 is still raising an stack overflow error). @lustefaniak I could decrease it here as default value for logic https://github.com/getsynq/sqlparser-rs/blob/main/src/parser/mod.rs#L200, but not sure about it.

What is your advice?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to decrease it in the default, as that is the one used by the gRPC service, if the code is going to fail otherwise we need to make sure it works and cleanly reports failure instead of crashing the app.

@zdenal zdenal merged commit b9f683c into main Mar 5, 2024
2 checks passed
@lustefaniak lustefaniak deleted the zdenko/syn-3024-bigquery-parse-errro-partition-by branch March 26, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants