airflow icon indicating copy to clipboard operation
airflow copied to clipboard

Fix BigQueryCursor execute method if the location is missing

Open e-galan opened this issue 9 months ago • 4 comments

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.

e-galan avatar May 16 '24 08:05 e-galan

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

e-galan avatar May 16 '24 10:05 e-galan

This static check failure not relevant your PR, it will be fix (at least temporary) by https://github.com/apache/airflow/pull/39662

Taragolis avatar May 16 '24 10:05 Taragolis

@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?

eladkal avatar May 18 '24 08:05 eladkal

@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

VladaZakharova avatar May 20 '24 10:05 VladaZakharova

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

e-galan avatar May 21 '24 17:05 e-galan

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

VladaZakharova avatar May 23 '24 09:05 VladaZakharova