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 · 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.

tswast · 2025-02-06T18:53:10Z

google/cloud/bigquery/_helpers.py

@@ -978,10 +978,12 @@ def _build_resource_from_properties(obj, filter_fields):
    """
    partial = {}
    for filter_field in filter_fields:
-        api_field = obj._PROPERTY_TO_API_FIELD.get(filter_field)
+        api_field = _get_sub_prop(obj._PROPERTY_TO_API_FIELD, filter_field)


I'm a bit confused with this part. The key is the property name of the class (e.g. what is the name of the @property in the Table class). I don't think that should ever by a list.

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))

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

Adds logic to detect non-Sequence schema.fields value

89e3a7d

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

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

Linchin Feb 4, 2025

chalmerlowe Feb 6, 2025

tswast Feb 6, 2025

tswast Feb 6, 2025

	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

Are you sure you want to change the base?

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

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