-
Notifications
You must be signed in to change notification settings - Fork 65
feat: Add support for Proto and Enum types #1202
Changes from 2 commits
c85c31e
9132e80
f65d45a
e1fac0c
13236e6
d785b8b
7dea490
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,8 +11,13 @@ | |
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| from __future__ import annotations | ||
|
|
||
| from typing import Any, Callable, Dict, Type | ||
| from typing import Any, Callable, Dict, Type, Union, Optional | ||
| from typing_extensions import TypeAlias | ||
|
|
||
| from google.protobuf.message import Message | ||
| from google.protobuf.internal.enum_type_wrapper import EnumTypeWrapper | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this safe to use? (I see internal in there)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| from google.cloud.bigtable.data.execute_query.values import Struct | ||
| from google.cloud.bigtable.data.execute_query.metadata import SqlType | ||
| from google.cloud.bigtable_v2 import Value as PBValue | ||
|
|
@@ -30,24 +35,36 @@ | |
| SqlType.Struct: "array_value", | ||
| SqlType.Array: "array_value", | ||
| SqlType.Map: "array_value", | ||
| SqlType.Proto: "bytes_value", | ||
| SqlType.Enum: "int_value", | ||
| } | ||
|
|
||
|
|
||
| def _parse_array_type(value: PBValue, metadata_type: SqlType.Array) -> Any: | ||
| def _parse_array_type( | ||
| value: PBValue, | ||
| metadata_type: SqlType.Array, | ||
| column_name: str | None, | ||
| column_info: dict[str, Union[Message, EnumTypeWrapper]] | None = None, | ||
| ) -> Any: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know you didn't touch this part, but maybe this should be |
||
| """ | ||
| used for parsing an array represented as a protobuf to a python list. | ||
| """ | ||
| return list( | ||
| map( | ||
| lambda val: _parse_pb_value_to_python_value( | ||
| val, metadata_type.element_type | ||
| val, metadata_type.element_type, column_name, column_info | ||
| ), | ||
| value.array_value.values, | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| def _parse_map_type(value: PBValue, metadata_type: SqlType.Map) -> Any: | ||
| def _parse_map_type( | ||
| value: PBValue, | ||
| metadata_type: SqlType.Map, | ||
| column_name: str | None, | ||
| column_info: dict[str, Union[Message, EnumTypeWrapper]] | None = None, | ||
| ) -> Any: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above: |
||
| """ | ||
| used for parsing a map represented as a protobuf to a python dict. | ||
|
|
||
|
|
@@ -64,10 +81,16 @@ def _parse_map_type(value: PBValue, metadata_type: SqlType.Map) -> Any: | |
| map( | ||
| lambda map_entry: ( | ||
| _parse_pb_value_to_python_value( | ||
| map_entry.array_value.values[0], metadata_type.key_type | ||
| map_entry.array_value.values[0], | ||
| metadata_type.key_type, | ||
| f"{column_name}.key" if column_name is not None else None, | ||
| column_info, | ||
| ), | ||
| _parse_pb_value_to_python_value( | ||
| map_entry.array_value.values[1], metadata_type.value_type | ||
| map_entry.array_value.values[1], | ||
| metadata_type.value_type, | ||
| f"{column_name}.value" if column_name is not None else None, | ||
| column_info, | ||
| ), | ||
| ), | ||
| value.array_value.values, | ||
|
|
@@ -77,7 +100,12 @@ def _parse_map_type(value: PBValue, metadata_type: SqlType.Map) -> Any: | |
| raise ValueError("Invalid map entry - less or more than two values.") | ||
|
|
||
|
|
||
| def _parse_struct_type(value: PBValue, metadata_type: SqlType.Struct) -> Struct: | ||
| def _parse_struct_type( | ||
| value: PBValue, | ||
| metadata_type: SqlType.Struct, | ||
| column_name: str | None, | ||
| column_info: dict[str, Union[Message, EnumTypeWrapper]] | None = None, | ||
| ) -> Struct: | ||
| """ | ||
| used for parsing a struct represented as a protobuf to a | ||
| google.cloud.bigtable.data.execute_query.Struct | ||
|
|
@@ -88,29 +116,132 @@ def _parse_struct_type(value: PBValue, metadata_type: SqlType.Struct) -> Struct: | |
| struct = Struct() | ||
| for value, field in zip(value.array_value.values, metadata_type.fields): | ||
| field_name, field_type = field | ||
| struct.add_field(field_name, _parse_pb_value_to_python_value(value, field_type)) | ||
| nested_column_name: str | None | ||
| if column_name is None: | ||
| nested_column_name = None | ||
| else: | ||
| # qualify the column name for nested lookups | ||
| nested_column_name = ( | ||
| f"{column_name}.{field_name}" if field_name else column_name | ||
|
jackdingilian marked this conversation as resolved.
Outdated
|
||
| ) | ||
| struct.add_field( | ||
| field_name, | ||
| _parse_pb_value_to_python_value( | ||
| value, field_type, nested_column_name, column_info | ||
| ), | ||
| ) | ||
|
|
||
| return struct | ||
|
|
||
|
|
||
| def _parse_timestamp_type( | ||
| value: PBValue, metadata_type: SqlType.Timestamp | ||
| value: PBValue, | ||
| metadata_type: SqlType.Timestamp, | ||
| column_name: str | None, | ||
| column_info: dict[str, Union[Message, EnumTypeWrapper]] | None = None, | ||
| ) -> DatetimeWithNanoseconds: | ||
| """ | ||
| used for parsing a timestamp represented as a protobuf to DatetimeWithNanoseconds | ||
| """ | ||
| return DatetimeWithNanoseconds.from_timestamp_pb(value.timestamp_value) | ||
|
|
||
|
|
||
| _TYPE_PARSERS: Dict[Type[SqlType.Type], Callable[[PBValue, Any], Any]] = { | ||
| def _parse_proto_type( | ||
| value: PBValue, | ||
| metadata_type: SqlType.Proto, | ||
| column_name: str | None, | ||
| column_info: dict[str, Union[Message, EnumTypeWrapper]] | None = None, | ||
| ) -> Message | bytes: | ||
| """ | ||
| Parses a serialized protobuf message into a Message object using type information | ||
| provided in column_info. | ||
|
|
||
| Args: | ||
| value: The value to parse, expected to have a bytes_value attribute. | ||
| metadata_type: The expected SQL type (Proto). | ||
| column_name: The name of the column. | ||
| column_info: (Optional) A dictionary mapping column names to their | ||
| corresponding Protobuf Message classes. This information is used | ||
| to deserialize the raw bytes. | ||
|
|
||
| Returns: | ||
| A deserialized Protobuf Message object if parsing is successful. | ||
| If parsing fails for any reason, or if the required type information | ||
| is not found in column_info, the function returns the original | ||
| serialized data as bytes (value.bytes_value). This fallback | ||
| ensures that the raw data is still accessible. | ||
| """ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think some more context here could be helpful. It seems like it just falls back to value.bytes_value if parsing fails? Is that an expected state, or does that mean something went wrong?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! Yes, that's correct. If In either case, we fall back to return the bytes. This approach/design mirrors the spanner's implementation in googleapis/python-spanner#1084.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, yeah I was going to question if it would be better to raise an exception on parse failure, but that could go either way. If spanner does it this way, it's probably good to follow suit |
||
| if ( | ||
| column_name is not None | ||
| and column_info is not None | ||
| and column_info.get(column_name) is not None | ||
| ): | ||
| default_proto_message = column_info.get(column_name) | ||
| if isinstance(default_proto_message, Message): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder of all of these nested conditionals could be simplified by using exception handling instead? (i.e, just assume everything exists, and return value.bytes_value if a parsing exception is thrown)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to refactor the code using a try-except block as you described, but this got me into several mypy errors as below: I'm not super familiar with Python language, so I'd appreciate any further advice/suggestion you could provide!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, you could resolve that using That said, you might still have to do a None check for column_name, because passing This might be getting into "overly clever" territory though, the original if statements worked fine too. Exception handling was something that came to mind to consider as I was reviewing, but maybe it's not worth it here |
||
| proto_message = type(default_proto_message)() | ||
| proto_message.ParseFromString(value.bytes_value) | ||
| return proto_message | ||
| return value.bytes_value | ||
|
|
||
|
|
||
| def _parse_enum_type( | ||
| value: PBValue, | ||
| metadata_type: SqlType.Enum, | ||
| column_name: str | None, | ||
| column_info: dict[str, Union[Message, EnumTypeWrapper]] | None = None, | ||
| ) -> int | str: | ||
| """ | ||
| Parses an integer value into a Protobuf enum name string using type information | ||
| provided in column_info. | ||
|
|
||
| Args: | ||
| value: The value to parse, expected to have an int_value attribute. | ||
| metadata_type: The expected SQL type (Enum). | ||
| column_name: The name of the column. | ||
| column_info: (Optional) A dictionary mapping column names to their | ||
| corresponding Protobuf EnumTypeWrapper objects. This information | ||
| is used to convert the integer to an enum name. | ||
|
|
||
| Returns: | ||
| A string representing the name of the enum value if conversion is successful. | ||
| If conversion fails for any reason, such as the required EnumTypeWrapper | ||
| not being found in column_info, or if an error occurs during the name lookup | ||
| (e.g., the integer is not a valid enum value), the function returns the | ||
| original integer value (value.int_value). This fallback ensures the | ||
| raw integer representation is still accessible. | ||
| """ | ||
| if ( | ||
| column_name is not None | ||
| and column_info is not None | ||
| and column_info.get(column_name) is not None | ||
| ): | ||
| proto_enum = column_info.get(column_name) | ||
| if isinstance(proto_enum, EnumTypeWrapper): | ||
| return proto_enum.Name(value.int_value) | ||
| return value.int_value | ||
|
|
||
|
|
||
| ParserCallable: TypeAlias = Callable[ | ||
| [PBValue, Any, Optional[str], Optional[Dict[str, Union[Message, EnumTypeWrapper]]]], | ||
|
jackdingilian marked this conversation as resolved.
|
||
| Any, | ||
| ] | ||
|
|
||
| _TYPE_PARSERS: Dict[Type[SqlType.Type], ParserCallable] = { | ||
| SqlType.Timestamp: _parse_timestamp_type, | ||
| SqlType.Struct: _parse_struct_type, | ||
| SqlType.Array: _parse_array_type, | ||
| SqlType.Map: _parse_map_type, | ||
| SqlType.Proto: _parse_proto_type, | ||
| SqlType.Enum: _parse_enum_type, | ||
| } | ||
|
|
||
|
|
||
| def _parse_pb_value_to_python_value(value: PBValue, metadata_type: SqlType.Type) -> Any: | ||
| def _parse_pb_value_to_python_value( | ||
| value: PBValue, | ||
| metadata_type: SqlType.Type, | ||
| column_name: str | None, | ||
| column_info: dict[str, Union[Message, EnumTypeWrapper]] | None = None, | ||
| ) -> Any: | ||
| """ | ||
| used for converting the value represented as a protobufs to a python object. | ||
| """ | ||
|
|
@@ -126,7 +257,7 @@ def _parse_pb_value_to_python_value(value: PBValue, metadata_type: SqlType.Type) | |
|
|
||
| if kind in _TYPE_PARSERS: | ||
| parser = _TYPE_PARSERS[kind] | ||
| return parser(value, metadata_type) | ||
| return parser(value, metadata_type, column_name, column_info) | ||
| elif kind in _REQUIRED_PROTO_FIELDS: | ||
| field_name = _REQUIRED_PROTO_FIELDS[kind] | ||
| return getattr(value, field_name) | ||
|
|
||
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: other places in this library, we use
|instead of the Union (i.e.dict[str, Message | EnumTypeWrapper]). Doesn't really matter though