bigtable: add ValueBitmaskFilter for data client#17252
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ValueBitmaskFilter class to support filtering cells by a value bitmask, along with corresponding unit tests. The reviewer pointed out that the _to_dict method incorrectly returns an instantiated protobuf message (data_v2_pb2.ValueBitmask) instead of a plain dictionary, which is inconsistent with other filters and could cause a NameError. Suggestions were provided to return a plain dictionary representation and to update the unit tests accordingly.
| def _to_dict(self) -> dict[str, Any]: | ||
| """Converts the row filter to a dict representation.""" | ||
| return {"value_bitmask_filter": data_v2_pb2.ValueBitmask(mask=self.mask)} |
There was a problem hiding this comment.
The _to_dict method should return a plain dictionary representation of the filter rather than instantiating a protobuf message (data_v2_pb2.ValueBitmask). Returning a protobuf message inside _to_dict is inconsistent with other filters, can cause NameError if data_v2_pb2 is not imported in this module, and defeats the purpose of a dictionary representation.
Instead, return a nested dictionary {"mask": self.mask} for the value_bitmask_filter key.
| def _to_dict(self) -> dict[str, Any]: | |
| """Converts the row filter to a dict representation.""" | |
| return {"value_bitmask_filter": data_v2_pb2.ValueBitmask(mask=self.mask)} | |
| def _to_dict(self) -> dict[str, Any]: | |
| """Converts the row filter to a dict representation.""" | |
| return {"value_bitmask_filter": {"mask": self.mask}} |
| def test_to_dict(self): | ||
| from google.cloud.bigtable_v2.types import data as data_v2_pb2 | ||
|
|
||
| mask = b"\xaa" * 8 | ||
| row_filter = self._target_class()(mask) | ||
| expected = {"value_bitmask_filter": data_v2_pb2.ValueBitmask(mask=mask)} | ||
| assert row_filter._to_dict() == expected |
There was a problem hiding this comment.
Update the test to assert against the plain dictionary representation returned by _to_dict(), which avoids the need to import data_v2_pb2 locally.
| def test_to_dict(self): | |
| from google.cloud.bigtable_v2.types import data as data_v2_pb2 | |
| mask = b"\xaa" * 8 | |
| row_filter = self._target_class()(mask) | |
| expected = {"value_bitmask_filter": data_v2_pb2.ValueBitmask(mask=mask)} | |
| assert row_filter._to_dict() == expected | |
| def test_to_dict(self): | |
| mask = bytes([0xaa]) * 8 | |
| row_filter = self._target_class()(mask) | |
| expected = {"value_bitmask_filter": {"mask": mask}} | |
| assert row_filter._to_dict() == expected |
|
Hi @axyjo, Please could you file a new issue (or link to an existing issue) in https://github.com/googleapis/google-cloud-python/issues? |
|
I think the unit test failures are unrelated to this PR, taking a look. Meanwhile @axyjo can you address this review comment #17252 (review)? Thanks. |
|
The test issues should have been fixed on main, but you may need to update your branch |
|
Closing. Created #17567 to address the comments and pulled from main. |
4 participants
Fixes #17255