Re: [PR] Support all bq load job and ext table config options in GCSToBigQueryOperator [airflow]

["Copilot (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org