Skip to content
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

fix: add warning when encountering unknown field types #1989

Merged
merged 15 commits into from
Aug 13, 2024

Conversation

suzmue
Copy link
Contributor

@suzmue suzmue commented Aug 5, 2024

Fixes #1988

@suzmue suzmue requested review from a team as code owners August 5, 2024 21:14
@suzmue suzmue requested a review from tswast August 5, 2024 21:14
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigquery Issues related to the googleapis/python-bigquery API. labels Aug 5, 2024
converter = _CELLDATA_FROM_JSON.get(field.field_type, lambda value, _: value)
def default_converter(value, field):
warnings.warn(
"Unknown type '{}' for field '{}'.".format(field.field_type, field.name),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be explicit and say "Behavior reading this type is not officially supported and may change in future." or something scary along those lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before we mark this as "fixed", I think the inverse direction is needed too (where we write JSON from a record), which is used in methods like client.insert_rows() (though maybe that already fails when encountering unknown types?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Yeah looks like

# Unknown fields should not be silently dropped, include them. Since there
and
converter = _SCALAR_VALUE_TO_JSON_ROW.get(field.field_type)
are two points. The first there is no type info so probably just _scalar_field_to_json needs to be updated. Updated PR to include a warning for the write.

add warning for write and warn about future behavior changes
@@ -382,7 +378,16 @@ def _field_to_index_mapping(schema):


def _field_from_json(resource, field):
converter = _CELLDATA_FROM_JSON.get(field.field_type, lambda value, _: value)
def default_converter(value, field):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can move this function to outside, so we can reuse it for _scalar_field_to_json() as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up just moving the warning itself to a new function, since the converter in _scalar_field_to_json has a different function signature.

@Linchin Linchin self-requested a review August 12, 2024 22:21
@Linchin Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 13, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 13, 2024
@Linchin
Copy link
Contributor

Linchin commented Aug 13, 2024

I think the docs test isn't failing due to changes in this PR, it appears to relate to the docker image used. I will approve this PR and force merge.

LGTM.

@Linchin Linchin merged commit 8f5a41d into googleapis:main Aug 13, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a warning in the type mapping if no conversion function is found
4 participants