feat: adds the SerDeInfo class and tests by chalmerlowe · Pull Request #2108 · googleapis/python-bigquery

chalmerlowe · 2025-01-09T18:51:10Z

This PR adds the SerDeInfo (serializer/deserializer info) class and the associated tests, plus minor tweaks to support both of those changes.

Linchin · 2025-01-09T19:04:11Z

+class SerDeInfo:
+ """Serializer and deserializer information.
+ Args:
+ serializationLibrary (str): Required. Specifies a fully-qualified class


Suggested change
serializationLibrary (str): Required. Specifiesafully-qualifiedclass
serialization_library (str): Required. Specifiesafully-qualifiedclass

+1 to transforming to PEP-8 compliant snake_case

tswast

Overall approach looks great. A couple nits.

tswast · 2025-01-09T19:37:10Z

 from google.cloud.bigquery import _helpers
 from google.cloud.bigquery import standard_sql
+from google.cloud.bigquery._helpers import (
+ _isinstance_or_raise,


Nit: Use _helpers._isinstance_or_raise in the code instead.
Use import statements for packages and modules only, not for individual types, classes, or functions.
https://google.github.io/styleguide/pyguide.html#22-imports

This is great feedback. I had not realized that.

tswast · 2025-01-09T19:37:43Z

+ """Serializer and deserializer information.
+ Args:


Might need a blank line here for the docs renderer to work?
Suggested change
"""Serializeranddeserializerinformation.
Args:
"""Serializeranddeserializerinformation.
Args:

tswast · 2025-01-09T19:38:25Z

+class SerDeInfo:
+ """Serializer and deserializer information.
+ Args:
+ serializationLibrary (str): Required. Specifies a fully-qualified class


+1 to transforming to PEP-8 compliant snake_case

tswast · 2025-01-09T19:40:19Z

+from google.cloud.bigquery.schema import (
+ PolicyTagList,
+ SerDeInfo,
+)


Same nit here, regarding https://google.github.io/styleguide/pyguide.html#22-imports

Hmm. I attempted to correct this for both PolicyTagList and SerDeInfo.
Unfortunately, there is something about how PolicyTagList is handled in our code that did not like the change and it produced several failing tests. I did not want to clutter this PR with unrelated changes, nor did I want to delay the issuance of this PR.
So I resolved this in the following way:
I corrected SerDeInfo and throughout the code we reference schema.SerDeInfo.
I left the PolicyTagList code exactly as it was found.

tswast · 2025-01-09T19:41:58Z

 return answer
+
+
+class SerDeInfo:


Is this the name in the REST API? I'd prefer the full names rather than abbreviations, but I'm OK with this if it's to be consistent with the proto/REST API.

SerDeInfo is the name in the proto/REST API definitions.

Linchin · 2025-01-09T19:48:47Z

+
+ @property
+ def serialization_library(self) -> Any:
+ """Required. Specifies a fully-qualified class name of the serialization


nit: I think Required. should only apply to the init.

As mentioned in chat, I prefer to leave this as is.

Linchin · 2025-01-09T19:49:09Z

+
+ @property
+ def name(self) -> Any:
+ """Optional. Name of the SerDe. The maximum length is 256 characters."""


nit: similarly, we can remove Optional. here.

As mentioned in chat, I prefer to leave this as is.

Linchin · 2025-01-09T19:49:25Z

+ self._properties["serializationLibrary"] = value
+
+ @property
+ def name(self) -> Any:


Suggested change
defname(self) ->Any:
defname(self) ->Optional[str]:

Linchin · 2025-01-09T19:49:35Z

+ self.parameters = parameters
+
+ @property
+ def serialization_library(self) -> Any:


Suggested change
defserialization_library(self) ->Any:
defserialization_library(self) ->str:

Linchin · 2025-01-09T19:50:28Z

+ self._properties["name"] = value
+
+ @property
+ def parameters(self) -> Any:


Suggested change
defparameters(self) ->Any:
defparameters(self) ->Optional[dict[str, str]]:
For consistent type annotations.

Linchin · 2025-01-09T20:00:44Z

+ return self._properties.get("parameters")
+
+ @parameters.setter
+ def parameters(self, value: Optional[dict[str, str]] = None):


Same as https://github.com/googleapis/python-bigquery/pull/2108/files#r1909362061

See reply above from Python Docs about the concept of optional versus typing.Optional.

* feat: adds SerDeInfo class and tests * cleans up type hints and some minor tweaks

feat: adds SerDeInfo class and tests
840b5ea

chalmerlowe requested review from a team and agrawal-siddharth January 9, 2025 18:51

product-auto-labelBot added size: m Pull request size is medium. api: bigquery Issues related to the googleapis/python-bigquery API. labels Jan 9, 2025

blunderbuss-gcfBot assigned alvarowolfx Jan 9, 2025

chalmerlowe assigned tswast and Linchin and unassigned alvarowolfx Jan 9, 2025

Linchin reviewed Jan 9, 2025
View reviewed changes

tswast approved these changes Jan 9, 2025
View reviewed changes

Linchin reviewed Jan 9, 2025
View reviewed changes

Comment threadgoogle/cloud/bigquery/schema.py

Linchin reviewed Jan 9, 2025
View reviewed changes

cleans up type hints and some minor tweaks
de2b7a4

Linchin self-requested a review January 10, 2025 18:21

Linchin approved these changes Jan 10, 2025
View reviewed changes

chalmerlowe merged commit 62960f2 into mainJan 10, 2025

chalmerlowe deleted the feat-b358215039-adds-serdeinfo-class branch January 10, 2025 18:22

release-pleaseBot mentioned this pull request Jan 10, 2025
chore(main): release 3.28.0 #2056
Merged

This was referenced Jan 21, 2025
January 20, 2025 kitta65/bq-extension-vscode#639
Closed
January 20, 2025 kitta65/prettier-plugin-bq#579
Closed
January 20, 2025 kitta65/bq2cst#468
Closed

chalmerlowe added a commit that referenced this pull request Jan 22, 2025
feat: adds the SerDeInfo class and tests (#2108)
de45810
* feat: adds SerDeInfo class and tests * cleans up type hints and some minor tweaks

	serializationLibrary (str): Required. Specifiesafully-qualifiedclass
	serialization_library (str): Required. Specifiesafully-qualifiedclass

- """Serializeranddeserializerinformation.
-Args:
+ """Serializeranddeserializerinformation.
+Args:

	defserialization_library(self) ->Any:
	defserialization_library(self) ->str:

	defparameters(self) ->Any:
	defparameters(self) ->Optional[dict[str, str]]:

Uh oh!

Conversation

chalmerlowe commented Jan 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tswast left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalmerloweJan 9, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chalmerloweJan 9, 2025•
edited
Loading