feat: Add support for Proto and Enum types #1202

trollyxia · 2025-08-28T20:22:22Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

daniel-sanche

Here's a first pass, let me know what you think

daniel-sanche · 2025-09-04T23:51:23Z

google/cloud/bigtable/data/_async/client.py

            DeadlineExceeded,
            ServiceUnavailable,
        ),
+        column_info: dict[str, Any] | None = None,


Can we use something other than Any here?

It seems like the value is only used if it's a Message/EnumTypeWrapper?

daniel-sanche · 2025-09-04T23:56:57Z

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

+        and column_info.get(column_name) is not None
+    ):
+        default_proto_message = column_info.get(column_name)
+        if isinstance(default_proto_message, Message):


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)

I tried to refactor the code using a try-except block as you described, but this got me into several mypy errors as below:

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py:175: error: Item "None" of "Optional[Dict[str, Union[Message, EnumTypeWrapper]]]" has no attribute "get" [union-attr] google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py:175: error: Argument 1 to "get" of "dict" has incompatible type "Optional[str]"; expected "str" [arg-type] google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py:176: error: Missing positional argument "enum_type" in call to "EnumTypeWrapper" [call-arg] google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py:176: error: Cannot instantiate type "Type[None]" [misc] google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py:177: error: Item "EnumTypeWrapper" of "Union[Message, EnumTypeWrapper, Any]" has no attribute "ParseFromString" [union-attr] google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py:178: error: Incompatible return value type (got "Union[Message, EnumTypeWrapper, Any]", expected "Union[Message, bytes]") [return-value]

I'm not super familiar with Python language, so I'd appreciate any further advice/suggestion you could provide!

Hmm, you could resolve that using typing.cast, to tell mypy to assume it's a specific type:

try: proto_enum = cast(dict[str, EnumTypeWrapper], column_info).get(column_name) return cast(EnumTypeWrapper, proto_enum).Name(value.int_value) except AttributeError: return value.int_value

That said, you might still have to do a None check for column_name, because passing .get(None) doesn't seem to raise an exception

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

daniel-sanche · 2025-09-04T23:58:31Z

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

+) -> Message | bytes:
+    """
+    Parses a serialized protobuf message into a Message object.
+    """


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?

Done!

Yes, that's correct. If column_info is not provided or the provided column_info doesn't contain the required type information, then it is expected that we return value.bytes_value. However, if parsing fails then that does mean something went wrong (i.e., the user provided the wrong type information).

In either case, we fall back to return the bytes. This approach/design mirrors the spanner's implementation in googleapis/python-spanner#1084.

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

daniel-sanche · 2025-09-05T00:04:54Z

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

+    metadata_type: SqlType.Enum,
+    column_name: str | None,
+    column_info: dict[str, Any] | None = None,
+) -> int | Any:


does this have to be Any?

I looked up EnumTypeWrapper.Name, and it seems like it's supposed to be a string?

Good catch. Done!

daniel-sanche · 2025-09-05T00:06:26Z

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

+
+
+_TYPE_PARSERS: Dict[
+    Type[SqlType.Type], Callable[[PBValue, Any, str | None, dict[str, Any] | None], Any]


nit: it might be good to make a type alias for Callable[[PBValue, Any, str | None, dict[str, Any] | None], Any]. Maybe call it ParserCallable or something?

The usage of TypeAlias seems to break several checks :(

Could you please let me know what I've done wrong?

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:

ParserCallable = Callable[ [PBValue, Any, Optional[str], Optional[Dict[str, Union[Message, EnumTypeWrapper]]]], Any, ]

That seems to be what we did for other aliases in this library. Let me know if that causes any issues

daniel-sanche · 2025-09-05T00:16:06Z

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

 from typing import Any, Callable, Dict, Type
+
+from google.protobuf.message import Message
+from google.protobuf.internal.enum_type_wrapper import EnumTypeWrapper


is this safe to use? (I see internal in there)

python-spanner uses the same thing (https://github.com/search?q=repo%3Agoogleapis%2Fpython-spanner+google.protobuf.internal.enum_type_wrapper&type=code) so I'm assuming it should be fine.

daniel-sanche · 2025-09-05T00:25:54Z

google/cloud/bigtable/data/_async/client.py

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


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

I expanded the docstring. Hopefully it makes more sense now. Please take another look thanks!

That looks good to me!

daniel-sanche

Looking good! just responded to a couple more comments

daniel-sanche · 2025-09-05T21:20:04Z

google/cloud/bigtable/data/_async/client.py

            DeadlineExceeded,
            ServiceUnavailable,
        ),
+        column_info: dict[str, Union[Message, EnumTypeWrapper]] | None = None,


nit: other places in this library, we use | instead of the Union (i.e. dict[str, Message | EnumTypeWrapper]). Doesn't really matter though

daniel-sanche · 2025-09-05T21:23:50Z

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

+) -> Message | bytes:
+    """
+    Parses a serialized protobuf message into a Message object.
+    """


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

daniel-sanche · 2025-09-05T21:24:38Z

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

+    metadata_type: SqlType.Array,
+    column_name: str | None,
+    column_info: dict[str, Union[Message, EnumTypeWrapper]] | None = None,
+) -> Any:


I know you didn't touch this part, but maybe this should be list[Any]?

daniel-sanche · 2025-09-05T21:25:17Z

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

+    metadata_type: SqlType.Map,
+    column_name: str | None,
+    column_info: dict[str, Union[Message, EnumTypeWrapper]] | None = None,
+) -> Any:


same as above: dict[Any, Any]?

daniel-sanche · 2025-09-05T21:26:29Z

google/cloud/bigtable/data/_async/client.py

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


That looks good to me!

daniel-sanche · 2025-09-05T21:34:36Z

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

+
+
+_TYPE_PARSERS: Dict[
+    Type[SqlType.Type], Callable[[PBValue, Any, str | None, dict[str, Any] | None], Any]


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:

ParserCallable = Callable[ [PBValue, Any, Optional[str], Optional[Dict[str, Union[Message, EnumTypeWrapper]]]], Any, ]

That seems to be what we did for other aliases in this library. Let me know if that causes any issues

daniel-sanche · 2025-09-05T22:15:47Z

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

+        and column_info.get(column_name) is not None
+    ):
+        default_proto_message = column_info.get(column_name)
+        if isinstance(default_proto_message, Message):


Hmm, you could resolve that using typing.cast, to tell mypy to assume it's a specific type:

try: proto_enum = cast(dict[str, EnumTypeWrapper], column_info).get(column_name) return cast(EnumTypeWrapper, proto_enum).Name(value.int_value) except AttributeError: return value.int_value

That said, you might still have to do a None check for column_name, because passing .get(None) doesn't seem to raise an exception

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

daniel-sanche

LGTM

google/cloud/bigtable/data/_async/client.py

jackdingilian · 2025-09-09T15:31:51Z

google/cloud/bigtable/data/_async/client.py

+                    path from the top-level column name.
+
+                        - For STRUCTs: ``struct_column_name.field_name``
+                        - For MAPs: ``map_column_name.key`` or ``map_column_name.value`` to specify types


For the with_history maps that have nested struct values I assume this doesn't work? Or does it handle arbitrary nesting?

Ah cool it looks like this works. Can you add an example of that to the doc as well?

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

google/cloud/bigtable/data/_sync_autogen/client.py

google/cloud/bigtable/data/execute_query/_query_result_parsing_utils.py

google/cloud/bigtable/data/execute_query/metadata.py

tests/unit/data/execute_query/test_query_result_parsing_utils.py

…ailed

trollyxia requested review from a team as code owners August 28, 2025 20:22

trollyxia requested a review from msampathkumar August 28, 2025 20:22

product-auto-label bot added the size: xl Pull request size is extra large. label Aug 28, 2025

blunderbuss-gcf bot assigned daniel-sanche Aug 28, 2025

product-auto-label bot added the api: bigtable Issues related to the googleapis/python-bigtable API. label Aug 28, 2025

trollyxia force-pushed the proto branch from 623f32a to 5059bce Compare September 3, 2025 17:20

feat: Add support for Proto and Enum types

c85c31e

trollyxia force-pushed the proto branch from 5059bce to c85c31e Compare September 3, 2025 18:02

daniel-sanche reviewed Sep 5, 2025

View reviewed changes

Address review comments

9132e80

daniel-sanche reviewed Sep 5, 2025

View reviewed changes

trollyxia added 2 commits September 8, 2025 12:04

Minor change, address comments

f65d45a

Update docstring

e1fac0c

trollyxia force-pushed the proto branch from 80b6b6b to e1fac0c Compare September 8, 2025 17:16

daniel-sanche previously approved these changes Sep 8, 2025

View reviewed changes

daniel-sanche added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 8, 2025

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 8, 2025

jackdingilian reviewed Sep 10, 2025

View reviewed changes

Address review comments

13236e6

trollyxia dismissed daniel-sanche’s stale review via 13236e6 September 10, 2025 20:41

trollyxia added 2 commits September 11, 2025 12:00

Modify docstring to indicate message.DecodeError when proto parsing f…

d785b8b

…ailed

Disable proto/enum parsing for unnamed proto/enum fields inside a struct

7dea490

trollyxia force-pushed the proto branch from e7d0faa to 7dea490 Compare September 11, 2025 18:19

jackdingilian approved these changes Sep 11, 2025

View reviewed changes

daniel-sanche added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 11, 2025

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 11, 2025

daniel-sanche added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 11, 2025

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 11, 2025

daniel-sanche added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2025

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2025

daniel-sanche added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2025

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2025

daniel-sanche merged commit 34ceb86 into googleapis:main Sep 12, 2025
29 of 30 checks passed

release-please bot mentioned this pull request Aug 26, 2025

chore(main): release 2.33.0 #1185

Merged



		_TYPE_PARSERS: Dict[
		Type[SqlType.Type], Callable[[PBValue, Any, str \| None, dict[str, Any] \| None], Any]

feat: Add support for Proto and Enum types #1202

feat: Add support for Proto and Enum types #1202

Uh oh!

Conversation

trollyxia commented Aug 28, 2025

Uh oh!

daniel-sanche 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!

daniel-sanche Sep 5, 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!

trollyxia Sep 5, 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!

daniel-sanche 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!

daniel-sanche Sep 5, 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!

daniel-sanche left a comment

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!

Choose a reason for hiding this comment

Uh oh!

daniel-sanche Sep 5, 2025 •

edited

Loading

trollyxia Sep 5, 2025 •

edited

Loading

daniel-sanche Sep 5, 2025 •

edited

Loading