8000 Add autodetect parameter to BigQueryLoadTask (#2363) by mklimasz · Pull Request #2575 · spotify/luigi · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add autodetect parameter to BigQueryLoadTask (#2363) #2575

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

Jump to bottom
Merged
merged 2 commits into from
Jan 18, 2019
Merged

Add autodetect parameter to BigQueryLoadTask (#2363) #2575

merged 2 commits into from
Jan 18, 2019

Conversation

mklimasz
Copy link
Contributor
@mklimasz mklimasz commented Nov 8, 2018

Description

When schema is not provided try to use autodetect parameter.
Removed comment that implicates that it doesn't work for JSON
(docs linked below explicitly says it works for JSON)

Motivation and Context

It solves: #2363

Have you tested this? If so, how?

I ran my jobs with this code and it works for me.
It also matches description of configuration.load.autodetect
in docs https://cloud.google.com/bigquery/docs/reference/rest/v2/jobs#configuration.load

@mklimasz
Copy link
Contributor Author
mklimasz commented Nov 28, 2018 •
edited
Loading

Hey! Anybody will have time to check this one out or let's agree to close it? Regards.

I allow myself to notify you, @dlstadther

dlstadther
dlstadther reviewed Nov 28, 2018
View reviewed changes
Copy link
Collaborator
@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

Sorry - this fell through the cracks!

The code changes make sense to me.

It looks like you based off a broken version of master. Could you merge in master?

Also, not sure if this is something you can add a unittest for, but if so that'd be great!

Thanks!

@mklimasz
Copy link
Contributor Author

I have merged current master.

I don't think this change would be testable in unit tests, maybe in some integration tests.

dlstadther
dlstadther approved these changes Nov 28, 2018
View reviewed changes
Copy link
Collaborator
@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

Thanks

That's fine. It's only an else conditional anyway that's straightforward. I guess so long as it works for you in production, i'm good with it

Tarrasch
Tarrasch approved these changes Dec 2, 2018
View reviewed changes
Copy link
Contributor
@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe someone using GCP wants to comment?

@mklimasz
Copy link
Contributor Author

I guess nobody using GCP is keen on commenting, maybe we can merge it? :)

@dlstadther
Copy link
Collaborator

@honnix Do you/Spotify have BigQuery luigi experience to just say "LGTM"?

@honnix
Copy link
Member
honnix commented Jan 18, 2019

👍
This doc explains how it should work better I think: https://cloud.google.com/bigquery/docs/schema-detect

@dlstadther dlstadther merged commit bf9521f into spotify:master Jan 18, 2019
@mklimasz mklimasz mentioned this pull request Jan 18, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tarrasch Tarrasch Tarrasch approved these changes

@MarkChuCarroll MarkChuCarroll MarkChuCarroll approved these changes

@dlstadther dlstadther dlstadther approved these changes

@ulzha ulzha Awaiting requested review from ulzha

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mklimasz @dlstadther @honnix @Tarrasch @MarkChuCarroll
0