bigtable: add ValueBitmaskFilter for data client#17567
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the ValueBitmaskFilter class to support value bitmask row filters, along with corresponding unit tests. The review feedback correctly identifies that in the Bigtable V2 API, value_bitmask_filter is a direct bytes field rather than a nested message. Consequently, the _to_dict implementation and both unit tests (test_to_dict and test_to_pb) need to be updated to map the mask directly to value_bitmask_filter to avoid runtime errors and match the API schema.
| def _to_dict(self) -> dict[str, Any]: | ||
| """Converts the row filter to a dict representation.""" | ||
| return {"value_bitmask_filter": {"mask": self.mask}} |
There was a problem hiding this comment.
In the Bigtable V2 API, value_bitmask_filter is a bytes field, not a message with a mask field. Therefore, _to_dict should return {"value_bitmask_filter": self.mask} directly rather than nesting it under a "mask" key.
| def _to_dict(self) -> dict[str, Any]: | |
| """Converts the row filter to a dict representation.""" | |
| return {"value_bitmask_filter": {"mask": self.mask}} | |
| def _to_dict(self) -> dict[str, bytes]: | |
| """Converts the row filter to a dict representation.""" | |
| return {"value_bitmask_filter": self.mask} |
There was a problem hiding this comment.
I don't think this is right. The proto is a ValueBitMask message with a mask field.
| def test_to_dict(self): | ||
| mask = b"\xaa" * 8 | ||
| row_filter = self._target_class()(mask) | ||
| expected = {"value_bitmask_filter": {"mask": mask}} | ||
| assert row_filter._to_dict() == expected |
There was a problem hiding this comment.
Update the expected dictionary format to match the corrected _to_dict implementation where value_bitmask_filter maps directly to the bytes mask.
| def test_to_dict(self): | |
| mask = b"\xaa" * 8 | |
| row_filter = self._target_class()(mask) | |
| expected = {"value_bitmask_filter": {"mask": mask}} | |
| assert row_filter._to_dict() == expected | |
| def test_to_dict(self): | |
| mask = b"\xaa" * 8 | |
| row_filter = self._target_class()(mask) | |
| expected = {"value_bitmask_filter": mask} | |
| assert row_filter._to_dict() == expected |
| def test_to_pb(self): | ||
| mask = b"\xaa" * 8 | ||
| row_filter = self._target_class()(mask) | ||
| pb = row_filter._to_pb() | ||
| assert pb.value_bitmask_filter.mask == mask |
There was a problem hiding this comment.
Since value_bitmask_filter is a bytes field in the protobuf definition, accessing .mask on it will raise an AttributeError. Assert directly against pb.value_bitmask_filter.
| def test_to_pb(self): | |
| mask = b"\xaa" * 8 | |
| row_filter = self._target_class()(mask) | |
| pb = row_filter._to_pb() | |
| assert pb.value_bitmask_filter.mask == mask | |
| def test_to_pb(self): | |
| mask = b"\xaa" * 8 | |
| row_filter = self._target_class()(mask) | |
| pb = row_filter._to_pb() | |
| assert pb.value_bitmask_filter == mask |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ValueBitmaskFilter class to support filtering Bigtable cells using a bitmask condition, along with corresponding unit and system tests. The reviewer suggested improving the comparison logic in ValueBitmaskFilter by removing the redundant __ne__ method and using the class name directly in isinstance checks. Additionally, the reviewer recommended specifying row_keys in the system tests to avoid inefficient full table scans and prevent potential test flakiness.
…filters.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…sync.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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:
Fixes #<issue_number_goes_here> 🦕