Skip to content

Return GCS URIs from GoogleAdsToGcsOperator#61334

Merged
shahar1 merged 5 commits intoapache:mainfrom
Abhishekmishra2808:fix-google-ads-to-gcs-uri-consistency
Mar 4, 2026
Merged

Return GCS URIs from GoogleAdsToGcsOperator#61334
shahar1 merged 5 commits intoapache:mainfrom
Abhishekmishra2808:fix-google-ads-to-gcs-uri-consistency

Conversation

@Abhishekmishra2808
Copy link
Contributor

PR Update

  • Update GoogleAdsToGcsOperator.execute() return type from None to list[str]
  • Return a list containing the full gs:// URI of the uploaded GCS object
  • Add docstring documentation for the new return value
  • Update unit tests to validate the returned GCS URI format
  • Fix test BUCKET constant by removing the gs:// prefix

Related: #11323

@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Feb 1, 2026
Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

You're in the way, but there's some work to so :)
Also, please fix static checks using prek (instructions are available in the docs)

@shahar1
Copy link
Contributor

shahar1 commented Feb 2, 2026

  1. In a second thought, I now wonder whether we should introduce the unwrap_single parameter at all, as it already returns the expected result (list[str]). It could be a nice feature for the while, at least until we align the rest of the operators.
    I'll be happy for more opinions, for now I keep it as is.

  2. Following a recent incident, I'd rather merge this PR after ensuring that the system tests pass.
    @Abhishekmishra2808 if you're able to do so, I'll be happy if you could attach a screenshot that it runs succesfully for the modified operator. Otherwise, we'll wait for Google team or anyone else to run them.

CC: @VladaZakharova @MaksYermak

@shahar1 shahar1 force-pushed the fix-google-ads-to-gcs-uri-consistency branch from 2d84e84 to 83e9ed4 Compare February 6, 2026 08:47
- Update execute() return type from None to list[str]
- Return list containing full gs:// URI of uploaded file
- Add docstring documenting return value
- Update unit test to validate return value format
- Fix BUCKET test constant (removed gs:// prefix)
…erator

- Add unwrap_single keyword-only parameter to __init__ (default=False)
- Update execute() return type to str | list[str]
- Implement conditional return logic:
  - Returns str when unwrap_single=True
  - Returns list[str] by default (backward compatible)
- Add comprehensive docstring updates
- Add test_execute_with_unwrap_single() to validate new behavior
- Maintain backward compatibility with existing code

Follows the pattern used in PR apache#61284
@shahar1 shahar1 force-pushed the fix-google-ads-to-gcs-uri-consistency branch from 83e9ed4 to f3b5fc9 Compare February 7, 2026 06:43
@Abhishekmishra2808
Copy link
Contributor Author

@yuseok89 could you please test it also ?

Copy link
Contributor

@yuseok89 yuseok89 left a comment

Choose a reason for hiding this comment

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

@shahar1 @Abhishekmishra2808

I haven't used Google Ads before, so I couldn't go into as much detail as with other operator tests.
That said, I did manage to get GoogleAdsToGcsOperator working, so I'm sharing screenshots.

Test Screenshots

unwrap_single=False

Image

unwrap_single=True

Image

@shahar1
Copy link
Contributor

shahar1 commented Mar 3, 2026

@shahar1 @Abhishekmishra2808

I haven't used Google Ads before, so I couldn't go into as much detail as with other operator tests.
That said, I did manage to get GoogleAdsToGcsOperator working, so I'm sharing screenshots.

Test Screenshots

unwrap_single=True

Image

unwrap_single=False

Image

Thanks for running!
It looks like the effect is reversed though (when unwrap_single is False it's unwrapped).
Did you misplace the titles by chance? :)

@yuseok89
Copy link
Contributor

yuseok89 commented Mar 3, 2026

Thanks for running! It looks like the effect is reversed though (when unwrap_single is False it's unwrapped). Did you misplace the titles by chance? :)

Oh, you're right. That was my mistake 😅
Looking at the screenshot, the task names and behavior do match, and I've confirmed in my code that the task names and unwrap_single param are aligned.
I mixed up the screenshots when I moved them over.
I'll update that comment.
Good catch!

@yuseok89
Copy link
Contributor

yuseok89 commented Mar 4, 2026

@shahar1
I'm investigating the CI issue across these PRs(#62829 #62832 ).

@shahar1 shahar1 changed the title Return GCS URIs from GoogleAdsToGcsOperator for Issue #11323 Return GCS URIs from GoogleAdsToGcsOperator Mar 4, 2026
@shahar1 shahar1 merged commit 5ebc044 into apache:main Mar 4, 2026
90 checks passed
1Ninad pushed a commit to 1Ninad/airflow that referenced this pull request Mar 6, 2026
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.

4 participants