-
Notifications
You must be signed in to change notification settings - Fork 17.5k
Emit GCSToBigQueryOperator deprecation warning after rendering #70542
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -261,8 +261,6 @@ def __init__( | |
| self.configuration: dict[str, Any] = {} | ||
|
|
||
| # GCS config | ||
| if src_fmt_configs is None: | ||
| src_fmt_configs = {} | ||
| if time_partitioning is None: | ||
| time_partitioning = {} | ||
| if range_partitioning is None: | ||
|
|
@@ -272,9 +270,6 @@ def __init__( | |
| self.bucket = bucket | ||
| self.source_objects = source_objects | ||
| self.schema_object = schema_object | ||
|
|
||
| if schema_object_bucket is None: | ||
| schema_object_bucket = bucket | ||
| self.schema_object_bucket = schema_object_bucket | ||
|
|
||
| # BQ config | ||
|
|
@@ -305,16 +300,6 @@ def __init__( | |
|
|
||
| self.schema_update_options = schema_update_options | ||
| self.src_fmt_configs = src_fmt_configs | ||
| if src_fmt_configs: | ||
| warnings.warn( | ||
| "The 'src_fmt_configs' parameter is deprecated. Use 'extra_config' instead. " | ||
| "Note: 'extra_config' uses the fully-nested API structure, so format-specific " | ||
| "options must be nested under their parent key " | ||
| "(e.g., {'parquetOptions': {'enableListInference': True}} rather than " | ||
| "{'enableListInference': True}).", | ||
| AirflowProviderDeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| self.extra_config = extra_config | ||
| self.time_partitioning = time_partitioning | ||
| self.range_partitioning = range_partitioning | ||
|
|
@@ -359,7 +344,30 @@ def _handle_job_error(job: BigQueryJob | UnknownJob) -> None: | |
| if job.error_result: | ||
| raise AirflowException(f"BigQuery job {job.job_id} failed: {job.error_result}") | ||
|
|
||
| def _warn_on_deprecated_template_fields(self) -> None: | ||
| if self.src_fmt_configs: | ||
| warnings.warn( | ||
| "The 'src_fmt_configs' parameter is deprecated. Use 'extra_config' instead. " | ||
| "Note: 'extra_config' uses the fully-nested API structure, so format-specific " | ||
| "options must be nested under their parent key " | ||
| "(e.g., {'parquetOptions': {'enableListInference': True}} rather than " | ||
| "{'enableListInference': True}).", | ||
| AirflowProviderDeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
|
|
||
| def execute(self, context: Context): | ||
| # Template fields render after __init__, so defaults that depend on a template field | ||
| # (schema_object_bucket falls back to bucket) and the src_fmt_configs deprecation check | ||
| # 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: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Not a reason to go back — resolving before rendering was the bug — but if Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the log as suggested. |
||
| self.schema_object_bucket = self.bucket | ||
| # Not captured in the rendered-template view (it defaults after rendering), so log it. | ||
| self.log.info("schema_object_bucket not set, defaulting to bucket %s", self.bucket) | ||
| self._warn_on_deprecated_template_fields() | ||
|
|
||
| hook = BigQueryHook( | ||
| gcp_conn_id=self.gcp_conn_id, | ||
| location=self.location, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stacklevel=2is 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 byexecute, stacklevel 2 points atexecuteitself — 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
There was a problem hiding this comment.
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.