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

Remove Avro Schema description copying from bigquery_avro #2269

Merged
merged 8 commits into from
Nov 23, 2017

Conversation

idreeskhan
Copy link
Contributor

Description

Google BigQuery now has support by default for Schema description copying. Removing the existing description copying from bigquery_avro.py since this is now redundant and may cause issues/conflicts in the future.

The logic currently will overwrite Google-side logic to copy over descriptions.

Have you tested this? If so, how?

I ran my jobs with this code and it works for me.

@idreeskhan
Copy link
Contributor Author

idreeskhan commented Oct 25, 2017

Any advice on how to handle the last few flake8 errors? Don't seem to be related to anything I have done and I tried to clean most of them up.

./test/decorator_test.py:136:7: E742 ambiguous class definition 'I'
./test/parameter_test.py:876:13: E741 ambiguous variable name 'l'
./test/parameter_test.py:880:25: E741 ambiguous variable name 'l'

@idreeskhan
Copy link
Contributor Author

@ulzha FYI

@ulzha
Copy link
Contributor

ulzha commented Oct 26, 2017

Yeah. Hm. Seems like flake8 has arbitrarily started to apply new rules.

@ulzha
Copy link
Contributor

ulzha commented Oct 27, 2017

Thanks for cleaning up. Does the bigquery_gcloud_test succeed unchanged? (We seem to not be running the gcloud tests on Travis.)

I think avro dependencies are becoming unneeded in

luigi/tox.ini

Line 24 in 4801fec

py27-gcloud: avro
and perhaps somewhere else as well...

@idreeskhan
Copy link
Contributor Author

Good catch on the test. Seems like google doesn't propagate the table level description at the moment so I'll have to pull that back in 😢

@ulzha
Copy link
Contributor

ulzha commented Oct 30, 2017

@idreeskhan or just file a feature request to propagate description as well, and not pull it back in here? :]

@idreeskhan
Copy link
Contributor Author

idreeskhan commented Oct 31, 2017

@ulzha Yes, for sure! We are working on that but the timeline may not match ours since this has a short timeline for us. Will know more and update later in the week.

@idreeskhan
Copy link
Contributor Author

idreeskhan commented Nov 21, 2017

@ulzha This will not be fixed soon enough by Google. I have re-added the table description copying logic and verified that the current tests still pass. I kept the field level description copying out as that is properly handled by BQ.

@ulzha
Copy link
Contributor

ulzha commented Nov 23, 2017

Merging despite codecov diff complaints... Removing code is awesome.

@ulzha ulzha merged commit 57bf97e into spotify:master Nov 23, 2017
@Tarrasch
Copy link
Contributor

Removing code is awesome

I cannot agree more. Thanks @idreeskhan!

@idreeskhan idreeskhan deleted the idrees/bq-avro-cleanup branch December 13, 2017 18:49
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