- Notifications
You must be signed in to change notification settings - Fork322
feat: adds the SerDeInfo class and tests#2108
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
google/cloud/bigquery/schema.py Outdated
| classSerDeInfo: | ||
| """Serializer and deserializer information. | ||
| Args: | ||
| serializationLibrary (str): Required. Specifies a fully-qualified class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| serializationLibrary (str):Required.Specifiesafully-qualifiedclass | |
| serialization_library (str):Required.Specifiesafully-qualifiedclass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
+1 to transforming to PEP-8 compliantsnake_case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Resolved.
tswast left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Overall approach looks great. A couple nits.
google/cloud/bigquery/schema.py Outdated
| fromgoogle.cloud.bigqueryimport_helpers | ||
| fromgoogle.cloud.bigqueryimportstandard_sql | ||
| fromgoogle.cloud.bigquery._helpersimport ( | ||
| _isinstance_or_raise, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is great feedback. I had not realized that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Resolved.
| """Serializer and deserializer information. | ||
| Args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Might need a blank line here for the docs renderer to work?
| """Serializeranddeserializerinformation. | |
| Args: | |
| """Serializeranddeserializerinformation. | |
| Args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Resolved.
google/cloud/bigquery/schema.py Outdated
| classSerDeInfo: | ||
| """Serializer and deserializer information. | ||
| Args: | ||
| serializationLibrary (str): Required. Specifies a fully-qualified class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
+1 to transforming to PEP-8 compliantsnake_case
tests/unit/test_schema.py Outdated
| fromgoogle.cloud.bigquery.schemaimport ( | ||
| PolicyTagList, | ||
| SerDeInfo, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same nit here, regardinghttps://google.github.io/styleguide/pyguide.html#22-imports
chalmerloweJan 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm. I attempted to correct this for bothPolicyTagList andSerDeInfo.
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
SerDeInfoand throughout the code we referenceschema.SerDeInfo. - I left the PolicyTagList code exactly as it was found.
| returnanswer | ||
| classSerDeInfo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SerDeInfo is the name in the proto/REST API definitions.
| @property | ||
| defserialization_library(self)->Any: | ||
| """Required. Specifies a fully-qualified class name of the serialization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: I thinkRequired. should only apply to the init.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As mentioned in chat, I prefer to leave this as is.
| @property | ||
| defname(self)->Any: | ||
| """Optional. Name of the SerDe. The maximum length is 256 characters.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: similarly, we can removeOptional. here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As mentioned in chat, I prefer to leave this as is.
google/cloud/bigquery/schema.py Outdated
| self._properties["serializationLibrary"]=value | ||
| @property | ||
| defname(self)->Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| defname(self)->Any: | |
| defname(self)->Optional[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Resolved.
google/cloud/bigquery/schema.py Outdated
| self.parameters=parameters | ||
| @property | ||
| defserialization_library(self)->Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| defserialization_library(self)->Any: | |
| defserialization_library(self)->str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Resolved.
google/cloud/bigquery/schema.py Outdated
| self._properties["name"]=value | ||
| @property | ||
| defparameters(self)->Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| defparameters(self)->Any: | |
| defparameters(self)->Optional[dict[str,str]]: |
For consistent type annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Resolved.
Uh oh!
There was an error while loading.Please reload this page.
| returnself._properties.get("parameters") | ||
| @parameters.setter | ||
| defparameters(self,value:Optional[dict[str,str]]=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See reply above from Python Docs about the concept ofoptional versustyping.Optional.
62960f2 intomainUh oh!
There was an error while loading.Please reload this page.
* feat: adds SerDeInfo class and tests* cleans up type hints and some minor tweaks
This PR adds the SerDeInfo (serializer/deserializer info) class and the associated tests, plus minor tweaks to support both of those changes.