Skip to content

Conversation

@yu-iskw
Copy link
Contributor

@yu-iskw yu-iskw commented Jul 22, 2017

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

[AIRFLOW-1389] BigQueryOperator should support `createDisposition` - ASF JIRA

Description

BigQueryOperator should support createDisposition.
I have added a parameter for configuration.query.createDisposition.

@mention-bot
Copy link

@yu-iskw, thanks for your PR! By analyzing the history of the files in this pull request, we identified @criccomini, @mistercrunch and @mtagle to be potential reviewers.

@yu-iskw yu-iskw closed this Jul 22, 2017
@yu-iskw yu-iskw reopened this Jul 22, 2017
@codecov-io
Copy link

codecov-io commented Jul 22, 2017

Codecov Report

Merging #2470 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2470      +/-   ##
==========================================
+ Coverage   69.39%   69.43%   +0.04%     
==========================================
  Files         146      146              
  Lines       11349    11351       +2     
==========================================
+ Hits         7876     7882       +6     
+ Misses       3473     3469       -4
Impacted Files Coverage Δ
airflow/operators/sensors.py 67.36% <0%> (+0.22%) ⬆️
airflow/jobs.py 77.12% <0%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa64f37...b35f887. Read the comment docs.

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 25, 2017

@criccomini Can you review it, when you have time?

def run_query(
self, bql, destination_dataset_table = False,
write_disposition = 'WRITE_EMPTY',
create_disposition='CREATE_IF_NEEDED',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little worried about this. For non named-parameter calls, this would be a backwards incompatible change. I know it's ugly, but how would you feel about putting it at the end of the param list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point. I was thinking the same thing as well. Alright, I will put the new parameter to the backwards. Thanks for the comment.

bql,
destination_dataset_table=False,
write_disposition='WRITE_EMPTY',
create_disposition='CREATE_IF_NEEDED',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above.

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 27, 2017

@criccomini Thanks for the feedback. I have modified the order of parameters.

@yu-iskw yu-iskw closed this Jul 27, 2017
@yu-iskw yu-iskw reopened this Jul 27, 2017
@asfgit asfgit closed this in 6e26407 Jul 31, 2017
@criccomini
Copy link
Contributor

LGTM! Merged.

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 31, 2017

@criccomini Thank you for merging it!

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.

4 participants