-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Support BigQuery custom schema's for external data using CSV / NDJSON #3717
Merged
danawillow
merged 4 commits into
GoogleCloudPlatform:master
from
ffung:fix/bq_external_data_custom_schema_6693
Jul 9, 2020
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 don't totally follow why not reading back the external schema requires us to do ForceNew. Couldn't we still allow updating that field even while not detecting drift on it?
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.
The ForceNew is not required in terms of reading back the data, I just consider this better UX wrt the expectations of the user, e.g. if you change the schema, the most probable course I think is to recreate the table with that schema. Afaik you cannot change the schema of an existing table in place.
The
external_data_configuration.schema
is only used as an input parameter for creating the table, when we read back the table, this field is always empty. There is a computedschema
returned on top level, which reflects the effective schema of the created table, however this value is calculated by combining the schema provided here and any other field/type mappings it can infer by autodetection and/or inferred from thesource_uri_prefix
. I wanted to avoid having to determine ifexternal_data_configuration.schema
is accurately reflected in the computed schema and reimplement BQ's logic in doing so, so I just assume after creation this is successfully reflected, hence I just ignore this field by making sure there are no changes with what is configured.Perhaps there's a smarter way to do this?
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.
That makes sense. If it's not clear that it can be updated and users wouldn't necessarily expect it to be, we can leave it as
ForceNew
. Worst thing that happens is someone files an issue to ask for update support.