You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "olayiwola-gbenga (via GitHub)" <gi...@apache.org> on 2026/08/21 14:27:38 UTC
[PR] Add bid manager operators [airflow]
olayiwola-gbenga opened a new pull request, #71947:
URL: https://github.com/apache/airflow/pull/71947
Added query and report operators for Google Bid Manager
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: https://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
For user-facing UI changes, please attach before/after screenshots (or a short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add query and report inspection operators for Google Bid Manager [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 merged PR #71947:
URL: https://github.com/apache/airflow/pull/71947
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add query and report inspection operators for Google Bid Manager [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #71947:
URL: https://github.com/apache/airflow/pull/71947#discussion_r4306520398
##########
providers/google/src/airflow/providers/google/marketing_platform/hooks/bid_manager.py:
##########
@@ -116,12 +122,17 @@ def get_report(self, query_id: str, report_id: str) -> dict:
.execute(num_retries=self.num_retries)
)
- def list_reports(self, query_id: str) -> dict:
+ def list_reports(self, query_id: str) -> list[dict]:
Review Comment:
Public hook method: the return type changes from `dict` to `list[dict]`, so callers doing `hook.list_reports(qid)["reports"]` break on upgrade. Keep the pagination but `return {"reports": reports}`, or add a `.. warning::` breaking-change note to `providers/google/docs/changelog.rst`.
##########
providers/google/src/airflow/providers/google/marketing_platform/sensors/bid_manager.py:
##########
@@ -21,6 +21,7 @@
from collections.abc import Sequence
from typing import TYPE_CHECKING
+from airflow.exceptions import AirflowFailException
Review Comment:
Deprecated path on Airflow 3 (emits `DeprecatedImportWarning`). Use `from airflow.providers.common.compat.sdk import AirflowFailException`. Same in the sensor test.
##########
providers/google/src/airflow/providers/google/marketing_platform/operators/bid_manager.py:
##########
@@ -317,8 +545,10 @@ def execute(self, context: Context):
resource = hook.get_report(query_id=self.query_id, report_id=self.report_id)
status = resource.get("metadata", {}).get("status", {}).get("state")
- if resource and status not in ["DONE", "FAILED"]:
- raise AirflowException(f"Report {self.report_id} for query {self.query_id} is still running")
+ if status != "DONE":
Review Comment:
A `FAILED` report never becomes `DONE`, so retrying it is pointless. Raise `AirflowFailException` for `FAILED` (as the sensor does) and keep a retryable, more specific exception for in-progress states.
##########
providers/google/tests/unit/google/marketing_platform/sensors/test_bid_manager.py:
##########
@@ -30,15 +33,36 @@
class TestGoogleBidManagerRunQuerySensor:
@mock.patch(f"{MODULE_NAME}.GoogleBidManagerHook")
- @mock.patch(f"{MODULE_NAME}.BaseSensorOperator")
- def test_poke(self, mock_base_op, hook_mock):
+ def test_poke_done(self, hook_mock):
Review Comment:
Nit: these three tests differ only in input and expected result, so one `@pytest.mark.parametrize` test would do. Same for the two `DownloadReportOperator` status tests.
##########
providers/google/tests/unit/google/marketing_platform/hooks/test_bid_manager.py:
##########
@@ -93,14 +93,20 @@ def test_get_query(self, get_conn_mock):
@mock.patch("airflow.providers.google.marketing_platform.hooks.bid_manager.GoogleBidManagerHook.get_conn")
def test_list_queries(self, get_conn_mock):
- queries = ["test"]
- return_value = {"queries": queries}
- get_conn_mock.return_value.queries.return_value.list.return_value.execute.return_value = return_value
+ queries_page1 = [{"queryId": "q1"}]
+ queries_page2 = [{"queryId": "q2"}]
+ mock_request = mock.MagicMock()
Review Comment:
Nit: `mock.MagicMock(spec=HttpRequest)` (from `googleapiclient.http`), here and in `test_list_reports`.
##########
providers/google/tests/unit/google/marketing_platform/operators/test_bid_manager.py:
##########
@@ -426,65 +430,220 @@ def f():
object_name=REPORT_NAME + ".gz",
)
+ @mock.patch("airflow.providers.google.marketing_platform.operators.bid_manager.GoogleBidManagerHook")
+ def test_execute_failed_status_raises(self, mock_hook):
+ mock_hook.return_value.get_report.return_value = {
+ "metadata": {
+ "status": {"state": "FAILED"},
+ }
+ }
+ mock_context = {"task_instance": mock.Mock()}
+ op = GoogleBidManagerDownloadReportOperator(
+ query_id=QUERY_ID,
+ report_id=REPORT_ID,
+ bucket_name=BUCKET_NAME,
+ report_name=REPORT_NAME,
+ task_id="test_task",
+ )
+ with pytest.raises(AirflowException, match="is in state 'FAILED'. Expected 'DONE'"):
+ op.execute(context=mock_context)
+
+ @mock.patch("airflow.providers.google.marketing_platform.operators.bid_manager.GoogleBidManagerHook")
+ def test_execute_running_status_raises(self, mock_hook):
+ mock_hook.return_value.get_report.return_value = {
+ "metadata": {
+ "status": {"state": "RUNNING"},
+ }
+ }
+ mock_context = {"task_instance": mock.Mock()}
+ op = GoogleBidManagerDownloadReportOperator(
+ query_id=QUERY_ID,
+ report_id=REPORT_ID,
+ bucket_name=BUCKET_NAME,
+ report_name=REPORT_NAME,
+ task_id="test_task",
+ )
+ with pytest.raises(AirflowException, match="is in state 'RUNNING'. Expected 'DONE'"):
+ op.execute(context=mock_context)
+
class TestGoogleBidManagerRunQueryOperator:
@mock.patch("airflow.providers.google.marketing_platform.operators.bid_manager.GoogleBidManagerHook")
def test_execute(self, hook_mock):
Review Comment:
This PR doesn't change `RunQueryOperator`, so please revert this rewrite (same for `TestGoogleBidManagerCreateQueryOperator.test_execute`). Keep the `test_prepare_template` fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add bid manager operators [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #71947:
URL: https://github.com/apache/airflow/pull/71947#issuecomment-6037402584
> I am not sure i understand the question 'Who needs these four operators?' :) These operators cover existing API and can be used instead of just calling it directly in system test Why exactly it was marked as draft?
Mainly asking because the Operators in these PRs are literally just one operator per hook method. So wanted to ensure there is a real need of this and we don't just create 1 operator per 1 hook method without a reason.
I think I detailed it on why it was converted to draft in previous comment :)
> The PR also changes behavior the description doesn't mention: list_reports now returns list[dict] instead of the raw response, the sensor raises on FAILED where it returned True, and DownloadReportOperator now raises unless the status is DONE. Is there an issue or a user asking for the operators, and a reported problem behind the behavior changes? Converting to draft until there is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add bid manager operators [airflow]
Posted by "VladaZakharova (via GitHub)" <gi...@apache.org>.
VladaZakharova commented on PR #71947:
URL: https://github.com/apache/airflow/pull/71947#issuecomment-6035003678
I am not sure i understand the question 'Who needs these four operators?' :)
These operators cover existing API and can be used instead of just calling it directly in system test
Why exactly it was marked as draft?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add bid manager operators [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #71947:
URL: https://github.com/apache/airflow/pull/71947#issuecomment-5942550299
Who needs these four operators? Each `execute()` only calls an existing `GoogleBidManagerHook` method (`get_query`, `list_queries`, `get_report`, `list_reports`), which a `@task` already covers. The PR also changes behavior the description doesn't mention: `list_reports` now returns `list[dict]` instead of the raw response, the sensor raises on `FAILED` where it returned `True`, and `DownloadReportOperator` now raises unless the status is `DONE`. Is there an issue or a user asking for the operators, and a reported problem behind the behavior changes? Converting to draft until there is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add query and report inspection operators for Google Bid Manager [airflow]
Posted by "olayiwola-gbenga (via GitHub)" <gi...@apache.org>.
olayiwola-gbenga commented on PR #71947:
URL: https://github.com/apache/airflow/pull/71947#issuecomment-6054750841
Thanks for the feedback @kaxil, and apologies for the brief PR description, the details were in the commit message (`867394baba`) instead of the PR body. The PR title and description have been updated now.
On why these four operators (`GetQuery`, `ListQueries`, `GetReport`, `ListReports`) are being added: currently `bid_manager.py` only has operators to create, run, delete, and download, while `get_query`, `list_queries`, `get_report`, and `list_reports` only exist on `GoogleBidManagerHook`. Adding these four operators completes the Bid Manager operator set so users can list and inspect existing queries and reports directly in their Dags (for example, checking existing query definitions or listing/inspecting reports generated by scheduled queries before downloading them).
On the behavior changes, testing against the Bid Manager API surfaced three bugs:
- `list_queries` and `list_reports` didn't handle pagination (`nextPageToken`), so they only returned the first page. Updating `list_reports` to page through `.list_next()` meant collecting the `"reports"` items across pages into a `list[dict]`, which also matches `list_queries`.
- `GoogleBidManagerRunQuerySensor` was returning `True` when the report status was `FAILED`, causing the sensor task to succeed even when report generation failed.
- `GoogleBidManagerDownloadReportOperator` only raised an error if the status was not in `["DONE", "FAILED"]`, so a `FAILED` report passed the check and then crashed with a `KeyError` on `resource["metadata"]["googleCloudStoragePath"]` (which is only present when the report status is `DONE`).
Please let me know if keeping these fixes together is fine now that the PR description is updated, or if splitting the pagination and `FAILED` status fixes into a separate PR is preferred.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org