Re: [PR] Support all bq load job and ext table config options in GCSToBigQueryOperator [airflow]
shahar1 merged PR #64505: URL: https://github.com/apache/airflow/pull/64505 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Support all bq load job and ext table config options in GCSToBigQueryOperator [airflow]
mlauter commented on PR #64505: URL: https://github.com/apache/airflow/pull/64505#issuecomment-4372241014 > This PR looks like it's ready for review, so I'm marking it as such and adding the `ready for maintainer review` label. > > As a friendly reminder — next time please mark your PR as ready for review yourself when you're done working on it. This helps maintainers find PRs that need attention more quickly. Thank you! 🙏 Oh sorry @potiuk I had been talking with @shahar1 about this directly, but i know he's had some stuff going on and i didn't want to be annoying. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Support all bq load job and ext table config options in GCSToBigQueryOperator [airflow]
Copilot commented on code in PR #64505:
URL: https://github.com/apache/airflow/pull/64505#discussion_r3066477987
##
providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_bigquery.py:
##
@@ -289,6 +303,17 @@ 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,
+)
Review Comment:
The deprecation warning only fires when `src_fmt_configs` is truthy. If a
DAG explicitly passes an empty dict (`src_fmt_configs={}`), this won’t warn
even though the deprecated parameter is used. Consider tracking whether the
argument was provided (e.g., `src_fmt_configs_provided = src_fmt_configs is not
None` before normalizing `None` to `{}`) and warning whenever it was provided,
regardless of emptiness.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Support all bq load job and ext table config options in GCSToBigQueryOperator [airflow]
potiuk commented on PR #64505: URL: https://github.com/apache/airflow/pull/64505#issuecomment-4195361097 This PR looks like it's ready for review, so I'm marking it as such and adding the `ready for maintainer review` label. As a friendly reminder — next time please mark your PR as ready for review yourself when you're done working on it. This helps maintainers find PRs that need attention more quickly. Thank you! 🙏 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Support all bq load job and ext table config options in GCSToBigQueryOperator [airflow]
mlauter commented on code in PR #64505:
URL: https://github.com/apache/airflow/pull/64505#discussion_r3040701992
##
providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_bigquery.py:
##
@@ -570,11 +595,15 @@ def _create_external_table(self):
)
external_config_api_repr[src_fmt_to_param_mapping[self.source_format]] =
self.src_fmt_configs
-external_config =
ExternalConfig.from_api_repr(external_config_api_repr)
if self.schema_fields:
-external_config.schema = [SchemaField.from_api_repr(f) for f in
self.schema_fields]
+external_config_api_repr["schema"] = {"fields": self.schema_fields}
if self.max_bad_records:
-external_config.max_bad_records = self.max_bad_records
+external_config_api_repr["maxBadRecords"] = self.max_bad_records
+
+if self.extra_config:
+external_config_api_repr.update(self.extra_config)
+
+external_config =
ExternalConfig.from_api_repr(external_config_api_repr)
Review Comment:
moving this below so that extra_config takes precedence over schema_fields
and max_bad_records
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Support all bq load job and ext table config options in GCSToBigQueryOperator [airflow]
mlauter commented on code in PR #64505: URL: https://github.com/apache/airflow/pull/64505#discussion_r3040694097 ## providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_bigquery.py: ## @@ -31,11 +32,11 @@ ExtractJob, LoadJob, QueryJob, -SchemaField, Review Comment: no longer needed because we're just including the schema fields in the main ExternalConfig dict under the `schema` key and calling `from_api_repr` on the whole thing -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Support all bq load job and ext table config options in GCSToBigQueryOperator [airflow]
mlauter commented on code in PR #64505: URL: https://github.com/apache/airflow/pull/64505#discussion_r3040495199 ## providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_bigquery.py: ## @@ -289,6 +301,13 @@ def __init__( self.schema_update_options = schema_update_options self.src_fmt_configs = src_fmt_configs +if src_fmt_configs: Review Comment: I disagree with this, no need to warn if someone passes an empty dict -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Support all bq load job and ext table config options in GCSToBigQueryOperator [airflow]
Copilot commented on code in PR #64505:
URL: https://github.com/apache/airflow/pull/64505#discussion_r3025329810
##
providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_bigquery.py:
##
@@ -570,6 +589,9 @@ def _create_external_table(self):
)
external_config_api_repr[src_fmt_to_param_mapping[self.source_format]] =
self.src_fmt_configs
+if self.extra_config:
+external_config_api_repr.update(self.extra_config)
+
external_config =
ExternalConfig.from_api_repr(external_config_api_repr)
if self.schema_fields:
external_config.schema = [SchemaField.from_api_repr(f) for f in
self.schema_fields]
Review Comment:
`extra_config` is merged into `external_config_api_repr` before
`ExternalConfig` is instantiated, but later in this method
`external_config.schema` and `external_config.max_bad_records` are set from
top-level params. That means `extra_config` does **not** consistently take
precedence for overlapping fields (e.g. `extra_config={"maxBadRecords": 10}`
will be overridden when `max_bad_records` is set). To preserve the documented
precedence, apply `extra_config` last (after all top-level-derived fields are
applied) or only set `schema`/`max_bad_records` when the corresponding key is
not present in `extra_config`.
##
providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_bigquery.py:
##
@@ -289,6 +301,13 @@ def __init__(
self.schema_update_options = schema_update_options
self.src_fmt_configs = src_fmt_configs
+if src_fmt_configs:
Review Comment:
The deprecation warning for `src_fmt_configs` is guarded by `if
src_fmt_configs:`. Because `src_fmt_configs` is normalized to `{}` when `None`
(and an explicitly provided empty dict is falsy), using the deprecated
parameter can fail to emit a warning. If the intent is to warn whenever the
parameter is provided, capture the original argument before defaulting and
check `src_fmt_configs is not None` (or use a sentinel) rather than a
truthiness check.
```suggestion
if src_fmt_configs is not None:
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
