feat(bigtable): Rerouted RowSet and RowRange to use ReadRows from data client - #18196
feat(bigtable): Rerouted RowSet and RowRange to use ReadRows from data client#18196daniel-sanche wants to merge 2 commits into
Conversation
**Changes made:** - Refactored `_MappableAttributesMixin` to `helpers.py` for use in `RowRange`. - Used `RowRange` and `ReadRowsQuery` from the data client as backing data sources for `RowRange` and `RowSet` respectively.
There was a problem hiding this comment.
Code Review
This pull request refactors RowSet and RowRange to use ReadRowsQuery and BaseRowRange from the data client, and extracts _MappableAttributesMixin into a shared helper module. The review feedback suggests addressing potential infinite recursion in _MappableAttributesMixin's magic methods and avoiding fragile private attribute access in RowRange.get_range_kwargs by using public properties.
| def __getattr__(self, name): | ||
| if name not in self._attribute_map: | ||
| raise AttributeError | ||
| return getattr(self, self._attribute_map[name]) | ||
|
|
||
| def __setattr__(self, name, value): | ||
| attribute = self._attribute_map.get(name, name) | ||
| super(_MappableAttributesMixin, self).__setattr__(attribute, value) |
There was a problem hiding this comment.
Accessing self._attribute_map inside __getattr__ can lead to infinite recursion (maximum recursion depth exceeded) if _attribute_map is not defined on the class or instance, or if it is accessed before initialization. To prevent this, add a guard to check if the requested attribute name is _attribute_map and raise an AttributeError immediately. Similarly, guard __setattr__ to handle _attribute_map safely.
| def __getattr__(self, name): | |
| if name not in self._attribute_map: | |
| raise AttributeError | |
| return getattr(self, self._attribute_map[name]) | |
| def __setattr__(self, name, value): | |
| attribute = self._attribute_map.get(name, name) | |
| super(_MappableAttributesMixin, self).__setattr__(attribute, value) | |
| def __getattr__(self, name): | |
| if name == "_attribute_map": | |
| raise AttributeError | |
| if name not in self._attribute_map: | |
| raise AttributeError | |
| return getattr(self, self._attribute_map[name]) | |
| def __setattr__(self, name, value): | |
| if name == "_attribute_map": | |
| super(_MappableAttributesMixin, self).__setattr__(name, value) | |
| return | |
| attribute = self._attribute_map.get(name, name) | |
| super(_MappableAttributesMixin, self).__setattr__(attribute, value) |
| def get_range_kwargs(self): | ||
| """Convert row range object to dict which can be passed to | ||
| google.bigtable.v2.RowRange add method. | ||
| """ | ||
| range_kwargs = {} | ||
| if self.start_key is not None: | ||
| start_key_key = "start_key_open" | ||
| if self.start_inclusive: | ||
| start_key_key = "start_key_closed" | ||
| range_kwargs[start_key_key] = _to_bytes(self.start_key) | ||
|
|
||
| if self.end_key is not None: | ||
| end_key_key = "end_key_open" | ||
| if self.end_inclusive: | ||
| end_key_key = "end_key_closed" | ||
| range_kwargs[end_key_key] = _to_bytes(self.end_key) | ||
| return range_kwargs | ||
| return { | ||
| descriptor.name: value for descriptor, value in self._pb._pb.ListFields() | ||
| } |
There was a problem hiding this comment.
Accessing the private attributes self._pb._pb is fragile and violates encapsulation. Since RowRange inherits from BaseRowRange (which exposes public properties start_key, end_key, start_is_inclusive, and end_is_inclusive), we can construct the dictionary using these public properties. This is safer, more robust, and avoids relying on the internal protobuf implementation details.
| def get_range_kwargs(self): | |
| """Convert row range object to dict which can be passed to | |
| google.bigtable.v2.RowRange add method. | |
| """ | |
| range_kwargs = {} | |
| if self.start_key is not None: | |
| start_key_key = "start_key_open" | |
| if self.start_inclusive: | |
| start_key_key = "start_key_closed" | |
| range_kwargs[start_key_key] = _to_bytes(self.start_key) | |
| if self.end_key is not None: | |
| end_key_key = "end_key_open" | |
| if self.end_inclusive: | |
| end_key_key = "end_key_closed" | |
| range_kwargs[end_key_key] = _to_bytes(self.end_key) | |
| return range_kwargs | |
| return { | |
| descriptor.name: value for descriptor, value in self._pb._pb.ListFields() | |
| } | |
| def get_range_kwargs(self): | |
| """Convert row range object to dict which can be passed to | |
| google.bigtable.v2.RowRange add method. | |
| """ | |
| range_kwargs = {} | |
| if self.start_key is not None: | |
| key = "start_key_closed" if self.start_is_inclusive else "start_key_open" | |
| range_kwargs[key] = self.start_key | |
| if self.end_key is not None: | |
| key = "end_key_closed" if self.end_is_inclusive else "end_key_open" | |
| range_kwargs[key] = self.end_key | |
| return range_kwargs |
Migrating over @gkevinzheng PR from bigtable monorepo googleapis/python-bigtable#1296
Original description:
Note to reviewers: This PR has already been reviewed and merged to a staging branch, with the intention of doing a single merge to main. We are now planning to slowly rollout these changes back to the main branch. Minimal re-review should be necessary