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 BigQueryCursor execute method if the location is missing #39659

Conversation

e-galan
Copy link
Contributor

@e-galan e-galan commented May 16, 2024

This addresses a bug where BigQueryValueCheckOperator tasks failed if it was not provided with the location parameter and the dataset location was outside of the US.

If the location parameter is not provided to BigQueryValueCheckOperator, then the location parameter will still be None when execution reaches the BigQueryCursor execute method. The execute method makes two client requests: the first to the BigQuery client, which can work without the location provided, the second to the Google Discovery client, which cannot find a resource without the location resulting in a error.

Pulling the location from a successful BigQuery request result resolves that problem.

Change list:

  • Make BigQueryCursor use the job location in the case when the location is not provided during the cursor's instantiation.
  • Update typing and docstrings.

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels May 16, 2024
@e-galan e-galan force-pushed the fix-bigquery-value-check-operator_job_location_bug branch from d6bd308 to a8b8013 Compare May 16, 2024 09:52
Copy link
Collaborator

@dirrao dirrao left a comment

Choose a reason for hiding this comment

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

is it feasible to add the test case for the bug?
Can you fix the failing static checks?

@e-galan
Copy link
Contributor Author

e-galan commented May 16, 2024

is it feasible to add the test case for the bug? Can you fix the failing static checks?

@dirrao I can add a unit test, but it will be totally dependent on mocks, because the problem happens between the two client requests. Will that do?

I can't fix the static check, it fails because of smth that does not seem related to my PR:
image

@Taragolis
Copy link
Contributor

This static check failure not relevant your PR, it will be fix (at least temporary) by #39662

@eladkal eladkal force-pushed the fix-bigquery-value-check-operator_job_location_bug branch from a8b8013 to 2cd6917 Compare May 18, 2024 06:58
@eladkal
Copy link
Contributor

eladkal commented May 18, 2024

@dirrao I can add a unit test, but it will be totally dependent on mocks, because the problem happens between the two client requests. Will that do?

We have system tests for google so probably we should modify/add coverage there. @VladaZakharova WDYT?

@VladaZakharova
Copy link
Contributor

@eladkal
Yes, thank you , will be good to add system test here to cover this scenario: f.e. try to create dataset and table with location different from US and then run BigQueryValueCheckOperator without specifying location.

@e-galan Please, add the changes accordingly

@e-galan e-galan force-pushed the fix-bigquery-value-check-operator_job_location_bug branch from 2cd6917 to 9f33610 Compare May 21, 2024 13:22
@e-galan
Copy link
Contributor Author

e-galan commented May 21, 2024

I have added the system test @dirrao @eladkal . Please take a look.

@e-galan e-galan requested a review from dirrao May 21, 2024 17:32
@VladaZakharova
Copy link
Contributor

Hi @eladkal @potiuk !
I guess we have made all the changes, can you please take a look again? :)

Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

LGTM

@eladkal eladkal merged commit 5aad588 into apache:main May 23, 2024
41 checks passed
RNHTTR pushed a commit to RNHTTR/airflow that referenced this pull request Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants