Skip to content

Emit GCSToBigQueryOperator deprecation warning after rendering - #70542

Open
yuseok89 wants to merge 2 commits into
apache:mainfrom
yuseok89:feature/70296-gcs-to-bigquery-warn-after-render
Open

Emit GCSToBigQueryOperator deprecation warning after rendering#70542
yuseok89 wants to merge 2 commits into
apache:mainfrom
yuseok89:feature/70296-gcs-to-bigquery-warn-after-render

Conversation

@yuseok89

Copy link
Copy Markdown
Contributor

related: #70296

src_fmt_configs is a template field but its deprecation warning and default were applied in __init__, before rendering. Move them (and the schema_object_bucket default) into execute() so they act on the rendered values, and drop the validate-operators-init exemption.


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)
    • Opus 4.8

  • Read the 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) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@boring-cyborg boring-cyborg Bot added area:dev-tools area:providers backport-to-v3-3-test Backport to v3-3-test provider:google Google (including GCP) related issues labels Jul 27, 2026
@yuseok89
yuseok89 force-pushed the feature/70296-gcs-to-bigquery-warn-after-render branch from 60f6716 to d5a886e Compare July 28, 2026 14:30
@yuseok89
yuseok89 marked this pull request as ready for review July 28, 2026 16:17

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks — this is the most substantial one in the campaign and it spots something the others don't. Moving the src_fmt_configs deprecation warning is routine, but schema_object_bucket falling back to bucket is a default that depends on a template field: computed in __init__, a templated bucket meant schema_object_bucket was silently set to the literal string "{{ ... }}" and the schema download went to a bucket named after un-rendered Jinja. That's a real bug, not just tidying.

I checked the src_fmt_configs is None -> {} normalisation survived the move — it did, into execute — since dropping it would have broken every downstream self.src_fmt_configs[...] access.

The explanatory comment in execute is good and exactly the kind of "why" note that earns its place. test_schema_object_bucket_defaults_to_bucket_when_omitted asserting download.assert_called_once_with(TEST_BUCKET, SCHEMA_OBJECT) pins the behaviour properly.

Two observations inline, neither blocking.


On those two inline points: I'd like you to think them through and reply with your own view rather than just applying or dismissing them. My review here was AI-assisted, and I'd guess parts of this PR may have been too — that's fine on both sides, but it means neither of us should treat the output as settled. The stacklevel point in particular is a judgement call that affects the whole campaign, not just this PR, so a considered human answer is worth more than a quick fix. Disagreeing with a reasoned explanation is a perfectly good outcome.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

# must run here, against the rendered values.
if self.src_fmt_configs is None:
self.src_fmt_configs = {}
if self.schema_object_bucket is None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Worth being aware of a side effect: because these defaults are now resolved after rendering, the rendered-template view in the UI records schema_object_bucket=None even though the run actually used bucket. Someone debugging a failed schema download will see None in the UI and the real bucket in the logs.

Not a reason to go back — resolving before rendering was the bug — but if schema_object_bucket is in template_fields, a log line at resolution time ("schema_object_bucket not set, defaulting to %s") would close the gap cheaply.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the log as suggested.
The observable change here is the RTIF None, which is fully restorable by moving the fallback into render_template_fields, but it'll recur for every operator in the campaign, so render-vs-log is probably worth deciding once campaign-wide rather than per-PR.

"(e.g., {'parquetOptions': {'enableListInference': True}} rather than "
"{'enableListInference': True}).",
AirflowProviderDeprecationWarning,
stacklevel=2,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

stacklevel=2 is now vestigial, and this applies across the whole campaign rather than just here.

When the warning fired from __init__ it pointed at the Dag author's line, which is what made it actionable. Emitted from a helper called by execute, stacklevel 2 points at execute itself — worker code the user can't act on — and there is no stack level that reaches the Dag file, because the Dag file isn't on the stack at execution time.

The deprecation is still visible in the task log, which is arguably enough. But since the parameter no longer does what it was there for, either dropping it or naming the offending task in the message ("task %s: the 'src_fmt_configs' parameter is deprecated...") would give users something to grep. Worth deciding once for the campaign rather than per-PR.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed it's vestigial.
Happy to follow whatever the campaign settles on.

@yuseok89
yuseok89 force-pushed the feature/70296-gcs-to-bigquery-warn-after-render branch from d5a886e to 65d407a Compare August 2, 2026 12:37
@eladkal eladkal removed the backport-to-v3-3-test Backport to v3-3-test label Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants