-
Notifications
You must be signed in to change notification settings - Fork 65
feat: Add support for Proto and Enum types #1202
Changes from 1 commit
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 |
|---|---|---|
|
|
@@ -657,6 +657,7 @@ async def execute_query( | |
| DeadlineExceeded, | ||
| ServiceUnavailable, | ||
| ), | ||
| column_info: dict[str, Any] | None = None, | ||
| ) -> "ExecuteQueryIteratorAsync": | ||
| """ | ||
| Executes an SQL query on an instance. | ||
|
|
@@ -705,6 +706,13 @@ async def execute_query( | |
| If None, defaults to prepare_operation_timeout. | ||
| prepare_retryable_errors: a list of errors that will be retried if encountered during prepareQuery. | ||
| Defaults to 4 (DeadlineExceeded) and 14 (ServiceUnavailable) | ||
| column_info: Dictionary with mappings between column names and additional column information. | ||
| An object where column names as keys and custom objects as corresponding | ||
| values for deserialization. It's specifically useful for data types like | ||
| protobuf where deserialization logic is on user-specific code. When provided, | ||
| the custom object enables deserialization of backend-received column data. | ||
| If not provided, data remains serialized as bytes for Proto Messages and | ||
| integer for Proto Enums. | ||
|
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. This is a bit confusing to me. will users know how to construct this dict? Is there a docs link we can add here or something? The first and second sentences also seem a bit contradictory, but maybe I'm reading it 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. I expanded the docstring. Hopefully it makes more sense now. Please take another look thanks!
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. That looks good to me! |
||
| Returns: | ||
| ExecuteQueryIteratorAsync: an asynchronous iterator that yields rows returned by the query | ||
| Raises: | ||
|
|
@@ -771,6 +779,7 @@ async def execute_query( | |
| attempt_timeout, | ||
| operation_timeout, | ||
| retryable_excs=retryable_excs, | ||
| column_info=column_info, | ||
| ) | ||
|
|
||
| @CrossSync.convert(sync_name="__enter__") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,8 +11,12 @@ | |
| # 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 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 +34,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, Any] | 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, Any] | 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 +80,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 +99,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, Any] | None = None, | ||
| ) -> Struct: | ||
| """ | ||
| used for parsing a struct represented as a protobuf to a | ||
| google.cloud.bigtable.data.execute_query.Struct | ||
|
|
@@ -88,29 +115,96 @@ 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, Any] | 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, Any] | None = None, | ||
| ) -> Message | bytes: | ||
| """ | ||
| Parses a serialized protobuf message into a Message object. | ||
| """ | ||
|
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, Any] | None = None, | ||
| ) -> int | 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. does this have to be Any? I looked up EnumTypeWrapper.Name, and it seems like it's supposed to be a string?
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. Good catch. Done! |
||
| """ | ||
| Parses an integer value into a Protobuf enum. | ||
| """ | ||
| 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 | ||
|
|
||
|
|
||
| _TYPE_PARSERS: Dict[ | ||
| Type[SqlType.Type], Callable[[PBValue, Any, str | None, dict[str, Any] | 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. nit: it might be good to make a type alias for
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.
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. The usage of Could you please let me know what I've done wrong?
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. Oh right, I don't think all versions we support work with TypeAlias I think you can just make it implicit by removing the TypeAlias annotation though: That seems to be what we did for other aliases in this library. Let me know if that causes any issues |
||
| ] = { | ||
| 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, Any] | None = None, | ||
| ) -> Any: | ||
| """ | ||
| used for converting the value represented as a protobufs to a python object. | ||
| """ | ||
|
|
@@ -126,7 +220,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.
Can we use something other than Any here?
It seems like the value is only used if it's a Message/EnumTypeWrapper?
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.
Done!