Warn when ignore_unknown_values is a no-op in GCSToBigQueryOperator - #71675
Warn when ignore_unknown_values is a no-op in GCSToBigQueryOperator#71675jonathangosling wants to merge 2 commits into
Conversation
Setting ignore_unknown_values=True has no effect under the default autodetect=True: the load job's schema is inferred from the same source data that the values are checked against, so no source field can be unknown to it. Users reach for the flag expecting fields absent from the destination table to be dropped, and instead hit a schema mismatch with no indication that the flag was ignored. The autodetect docstring made this harder to diagnose. It stated that the parameter must be True when no schema is supplied, and recommended True for tables created outside Airflow. Both steer users away from autodetect=None, which is the setting that makes ignore_unknown_values behave as expected against an existing table's schema.
boring-cyborg
Bot
added
area:providers
provider:google
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
SameerMesiah97
left a comment
SameerMesiah97
left a comment
There was a problem hiding this comment.
I have left some comments.
| * ``None`` - neither infer a schema nor supply one, so the destination table's own | ||
| schema is used. The table must already exist. This is the setting to use when you want | ||
| ``ignore_unknown_values`` to drop fields that are present in the source data but absent | ||
| from the destination table. |
There was a problem hiding this comment.
This is too long. There is no need to go deep into the interaction between this and the other parameters here. I would suggest the below instead:
:param autodetect: [Optional] Whether to infer the schema from the source data for CSV and JSON sources. If ``True``, the schema is inferred from the source data. If ``False``, either ``schema_fields`` or ``schema_object`` must be provided. If ``None``, no schema is supplied and the existing destination table's schema is used. (Default: ``True``).
There was a problem hiding this comment.
Good point, I've updated the docstring with your suggestion.
| "itself, so no source field can be unknown to it. To drop source fields that are " | ||
| "absent from an existing destination table, set `autodetect=None` so that the " | ||
| "destination table's own schema is used instead." | ||
| ) |
There was a problem hiding this comment.
- This helper is not needed as it is only called once. I would inline it.
- The warning is too verbose. I would suggest the below:
self.log.warning(
"`ignore_unknown_values` has no effect when `autodetect=True` and no schema is provided. "
"Set `autodetect=None` to use the existing destination table's schema instead."
)
There was a problem hiding this comment.
Updated:
- Inlined the check and warning.
- Made the message less verbose. I've used your suggestion, which covers the problem and the suggested fix more concisely.
| id="ignore_unknown_values_set_by_extra_config", | ||
| ), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Since autodetect is explicitly a three-state parameter, could we also cover autodetect=False here?
There was a problem hiding this comment.
I've added a test case for autodetect=False, supplying the schema_fields as required.
|
|
||
| assert mock_warning.call_args_list.count(call(IGNORE_UNKNOWN_VALUES_WARNING)) == ( | ||
| 1 if expects_warning else 0 | ||
| ) |
There was a problem hiding this comment.
Do these parametrized cases need to assert the complete warning text? The behaviour under test seems to be whether the warning is emitted. Using assert_called_once() / assert_not_called() would make these cases less coupled to the wording, with the message itself asserted in a single test if we want to protect it.
There was a problem hiding this comment.
Yes, good suggestion. I've updated to use assert_called_once() / assert_not_called(). On protecting the message, rather than pinning the full text, one test (test_ignore_unknown_values_no_op_warning_names_the_fix) now asserts only that the warning names autodetect=None, since that's the actionable part.
One thing worth flagging - these assertions don't discriminate which warning fired, so they'll break if an unrelated warning is added. Filtering on/asserting against a stable part of the message (i.e. ignore_unknown_values or autodetect=None), rather than the full message, could avoid that. Let me know if you think it's worth it.
Shorten the autodetect docstring to match the density of the surrounding parameters, and inline the single-use warning helper. Decouple the tests from the warning wording: the parametrized cases now assert only whether the warning fires, with a single test checking that it names the fix.
2 participants
ignore_unknown_values=Truesilently does nothing under the operator's defaultautodetect=True. The load job's schema is inferred from the same source data the values are checked against, so no source field can be unknown to it. Users set the flag expecting fields absent from the destination table to be dropped, and instead get a schema mismatch with nothing in the logs indicating the flag was ignored.This warns when the assembled load configuration has
autodetecttruthy,ignoreUnknownValuesset, and noschema, pointing atautodetect=None- the setting that makes BigQuery use the destination table's own schema.The check reads the assembled
configuration["load"]rather than the operator attributes, becausesrc_fmt_configsandextra_configcan override any of those three keys after they are set (all three are invalid_configsfor the relevant source formats). Two of the added test cases cover exactly that.It also fixes the
autodetectdocstring, which actively pointed the wrong way: it said the parameter "must be set to True if 'schema_fields' and 'schema_object' are undefined" (Noneis also valid, and is the documented way to load into an existing table), and that it is "suggested to set to True if table are create outside of Airflow" (for a table created outside Airflow,Trueis precisely what triggers the mismatch -Noneis what works). All three states are now documented, andautodetectgets abool | Noneannotation matching the existingis Falsecheck at the schema-object guard.Deliberately out of scope: changing the default, making
autodetect=Falsefall back to the table schema, and redesigning the flag. The three-state parameter is an artifact of"autodetect": self.autodetectbeing serialized unconditionally, soNoneis the only way to express "omit the field" - worth revisiting separately, but not in a bugfix PR.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines
{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.