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

fix: properly convert nested types to BQ schema #3185

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

plaflamme
Copy link
Contributor

This is related to #3184 but does not fix it. What it provides is a general function for converting a sqlglot schema to a bigquery schema. This new function is not in the codepath of altering schemas, but it should be possible to use it there to actually fix the issue.

@izeigerman
Copy link
Member

Thanks a lot for this, @plaflamme . Can this logic be tested at all?

@Themiscodes
Copy link
Contributor

Thank you @plaflamme! You're right when constructing the SchemaField we needed to traverse recursively to handle deeper levels of nesting correctly. And as you suggest changing:

bigquery.SchemaField(new_field[1], new_field[0], mode="NULLABLE")

to use your __get_bq_schemafield function:

self.__get_bq_schemafield(new_field[1], exp.DataType.build(new_field[0]))

does seem to resolve the issue when altering schemas, based on the examples I've tested so far.

@plaflamme
Copy link
Contributor Author

@izeigerman I didn't add tests since this is a private method, but we could extract it and test it on different exp.DataTypes, yes. As for the codepath that uses this, probably under test in the bigquery integration tests, but I'm not sure.

@Themiscodes great! I wasn't confident applying this new function was going to be sufficient to fix the issue as there are other things going on in when altering a schema. Glad to hear it's working out.

Copy link
Member

@izeigerman izeigerman left a comment

Choose a reason for hiding this comment

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

Great fix, @plaflamme , thank you!

@izeigerman izeigerman merged commit 42c3573 into TobikoData:main Sep 26, 2024
20 checks passed
@plaflamme plaflamme deleted the fix-nested-schema branch September 26, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants