Feat: Adds foreign_type_info attribute to table class and adds unit tests. #2126

chalmerlowe · 2025-02-04T13:19:42Z

This PR adds support for the foreign_type_info attribute on the Table class and the associated tests.

tswast · 2025-02-04T16:03:53Z

google/cloud/bigquery/table.py

+        For details, see:
+        https://cloud.google.com/bigquery/docs/external-tables
+        https://cloud.google.com/bigquery/docs/datasets-intro#external_datasets
+


I think we'll need some logic in the setter for schema to avoid overwriting the schema property entirely. Instead, it'll need to be responsible for just schema.fields.

Also, I'm not sure if this format will render well in the docs. We might just move all the contents under NOTE: to after Table's schema.

I have not yet addressed Tim's comment here.
I spoke with Linchin about the note and with the revision I added, Sphinx should be able to handle the note with no problem.

@tswast

fields and schema are two separate items:

two different attributes (with setters and getters)

two separate nodes on the .properties dict

It is possible for a user to supply an api_resource (a dict) that will overwrite both at the same time, such as: ._properties["schema"] = {fields: [], foreign_type_info: {type_system: "hello world"}. At that point, the end user should expect both to be overwritten.

Due to the nested separation of fields and schema in the ._properties dict and how we write content to it (either with setters OR directly into ._properties), I am not aware of any means where by setting one or the other will cause it's opposite to be accidentally overwritten.

I don't think any additional checks are required.

Also, in test_table.py::test_to_api_repr_w_schema_and_foreign_type_info test, it is broken into several steps. Two of those steps are specifically focused on ensuring that if either item is set the other does not change.

tswast · 2025-02-04T16:07:02Z

google/cloud/bigquery/table.py

+        https://cloud.google.com/bigquery/docs/datasets-intro#external_datasets
+        """
+
+        prop = self._properties.get(self._PROPERTY_TO_API_FIELD["foreign_type_info"])


Even if we are exposing this at the table level, it needs to be fetched from schema, still, right?

This is exactly the sort of thing the _get_sub_prop (

python-bigquery/google/cloud/bigquery/_helpers.py

Line 781 in 54c8d07

def _get_sub_prop(container, keys, default=None):

) and _set_sub_prop (

python-bigquery/google/cloud/bigquery/_helpers.py

Line 830 in 54c8d07

def _set_sub_prop(container, keys, value):

) helpers are intended to be used for.

We even use it in other Table properties, such as project:

python-bigquery/google/cloud/bigquery/table.py

Lines 175 to 177 in 54c8d07

return _helpers._get_sub_prop(

self._properties, self._PROPERTY_TO_API_FIELD["project"]

)

Suggested change

prop = self._properties.get(self._PROPERTY_TO_API_FIELD["foreign_type_info"])

prop = _helpers._get_sub_prop(self._properties, self._PROPERTY_TO_API_FIELD["foreign_type_info"])

tswast · 2025-02-04T16:09:18Z

google/cloud/bigquery/table.py

@@ -411,6 +412,7 @@ class Table(_TableBase):
        "max_staleness": "maxStaleness",
        "resource_tags": "resourceTags",
        "external_catalog_table_options": "externalCatalogTableOptions",
+        "foreign_type_info": "foreignTypeInfo",


We should adjust this to the format used for _helpers._get_sub_prop and _helpers._set_sub_prop. Likewise, let's replace schema with something compatible with that.

Suggested change

"foreign_type_info": "foreignTypeInfo",

"foreign_type_info": ["schema", "foreignTypeInfo"],

# TODO: remove "schema" from above (between time_partitioning and "snapshot_definition"

"schema": ["schema", "fields"],

Partially complete. Added a new value for schema, but not yet done with foreign_type_info.

tswast · 2025-02-04T16:11:04Z

tests/unit/test_table.py

@@ -5993,6 +5994,71 @@ def test_external_catalog_table_options_from_api_repr(self):
        assert result == expected


+class TestForeignTypeInfo:


I'd also like to see a test where we do Table.from_api_repr and Table.to_api_repr so that we can visually compare that the correct schema.foreignTypeInfo field of the REST API object is set.

Not yet addressed.

Linchin · 2025-02-04T23:16:07Z

tests/unit/test_table.py

+        table = self._make_one(self.TABLEREF)
+        assert table.foreign_type_info is None
+
+    def test_foreign_type_info_valid_inputs(self):


Could we also add test cases for the setter for other supported types, i.e., dict and None.

Not yet addressed.

Complete.
This test is now parametrized so that it tests under three input conditions:

dict

None

using a ForeignTypeInfo object

google/cloud/bigquery/_helpers.py

tswast · 2025-02-06T18:55:38Z

google/cloud/bigquery/_helpers.py

+            if isinstance(api_field, list):
+                api_field = api_field[0]
            partial[api_field] = obj._properties.get(api_field)


Maybe we should use _get_sub_prop and _set_sub_prop here? Since some overlap, I fear that the schema dictionary from schema.fields will overwrite the dictionary from schema.foreignTypeInfo or vice versa.

Suggested change

if isinstance(api_field, list):

api_field = api_field[0]

partial[api_field] = obj._properties.get(api_field)

_set_sub_prop(partial, api_field, _get_sub_prop(obj._properties, api_field))

tswast

Thanks!

Linchin

LGTM. Thank you!

chalmerlowe added 2 commits February 3, 2025 11:35

adds foreign_type_info attribute to table

7427b21

feat: Adds foreign_type_info attribute and tests

ee866b8

chalmerlowe requested review from a team as code owners February 4, 2025 13:19

chalmerlowe requested a review from hongalex February 4, 2025 13:19

product-auto-label bot added the size: m Pull request size is medium. label Feb 4, 2025

blunderbuss-gcf bot assigned PhongChuong Feb 4, 2025

product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Feb 4, 2025

Merge branch 'main' into feat-b358215039-adds-schema-mods

de4cfd5

chalmerlowe removed the request for review from hongalex February 4, 2025 13:20

chalmerlowe assigned tswast and Linchin and unassigned PhongChuong Feb 4, 2025

updates docstrings for foreign_type_info

3ee933b

tswast requested changes Feb 4, 2025

View reviewed changes

Linchin reviewed Feb 4, 2025

View reviewed changes

chalmerlowe added 2 commits February 6, 2025 18:20

Updates property handling, especially as regards set/get_sub_prop

5e6138e

Removes extraneous comments and debug expressions

fcd7f7b

tswast reviewed Feb 6, 2025

View reviewed changes

chalmerlowe added 3 commits February 7, 2025 05:32

Merge branch 'main' into feat-b358215039-adds-schema-mods

269ec23

Refactors build_resource_from_properties w get/set_sub_prop

a0f823e

updates to foreign_type_info, tests and wiring

e5ed4b8

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Feb 11, 2025

chalmerlowe added 3 commits February 12, 2025 11:50

Adds logic to detect non-Sequence schema.fields value

89e3a7d

updates assorted tests and logic

212c58e

Merge branch 'main' into feat-b358215039-adds-schema-mods

44f0447

tswast approved these changes Feb 20, 2025

View reviewed changes

Linchin self-requested a review February 20, 2025 18:35

Linchin approved these changes Feb 20, 2025

View reviewed changes

chalmerlowe enabled auto-merge (squash) February 21, 2025 16:46

chalmerlowe disabled auto-merge February 21, 2025 16:47

chalmerlowe enabled auto-merge (squash) February 21, 2025 17:10

chalmerlowe merged commit 2c19681 into main Feb 21, 2025
15 of 23 checks passed

chalmerlowe deleted the feat-b358215039-adds-schema-mods branch February 21, 2025 17:45

release-please bot mentioned this pull request Feb 21, 2025

chore(main): release 3.30.0 #2119

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Adds foreign_type_info attribute to table class and adds unit tests. #2126

Feat: Adds foreign_type_info attribute to table class and adds unit tests. #2126

chalmerlowe commented Feb 4, 2025

tswast Feb 4, 2025

Linchin Feb 4, 2025

chalmerlowe Feb 6, 2025

chalmerlowe Feb 20, 2025 •

edited

Loading

tswast Feb 4, 2025

chalmerlowe Feb 7, 2025

tswast Feb 4, 2025

chalmerlowe Feb 6, 2025

chalmerlowe Feb 7, 2025

tswast Feb 4, 2025

chalmerlowe Feb 6, 2025

chalmerlowe Feb 20, 2025

Linchin Feb 4, 2025

chalmerlowe Feb 6, 2025

chalmerlowe Feb 20, 2025

tswast Feb 6, 2025

chalmerlowe Feb 20, 2025

tswast left a comment

Linchin left a comment

	return _helpers._get_sub_prop(
	self._properties, self._PROPERTY_TO_API_FIELD["project"]
	)

	prop = self._properties.get(self._PROPERTY_TO_API_FIELD["foreign_type_info"])
	prop = _helpers._get_sub_prop(self._properties, self._PROPERTY_TO_API_FIELD["foreign_type_info"])

-        "foreign_type_info": "foreignTypeInfo",
+        "foreign_type_info": ["schema", "foreignTypeInfo"],
+        # TODO: remove "schema" from above (between time_partitioning and "snapshot_definition"
+        "schema": ["schema", "fields"],

		@@ -5993,6 +5994,71 @@ def test_external_catalog_table_options_from_api_repr(self):
		assert result == expected


		class TestForeignTypeInfo:

Feat: Adds foreign_type_info attribute to table class and adds unit tests. #2126

Feat: Adds foreign_type_info attribute to table class and adds unit tests. #2126

Conversation

chalmerlowe commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalmerlowe Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tswast left a comment

Choose a reason for hiding this comment

Linchin left a comment

Choose a reason for hiding this comment

chalmerlowe Feb 20, 2025 •

edited

Loading