feat: Rerouted ReadRows to data client - #18198
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 refactors the Bigtable client to delegate row reading and streaming to the underlying _table_impl data client, deprecating legacy classes like _RowMerger and several attributes on PartialRowsData. The review feedback identifies potential AttributeError risks when accessing retry.deadline directly (since google.api_core.retry.Retry typically uses _deadline internally) in table.py and unit tests, as well as when calling close() on _generator in row_data.py if the generator does not support it.
92da437 to
e435179
Compare
|
|
||
| self.rows = {} | ||
| @property | ||
| def last_scanned_row_key(self): |
There was a problem hiding this comment.
I think this property was only used in the internal method? Would it actually be called? Is it safe to just remove it?
Same question with read_method and retry, and request
There was a problem hiding this comment.
They were technically public attributes before, even though they weren't advertised. Hopefully they aren't used, but I thought the safer choice would be to raise a warning and return None when they are accessed, instead of letting the program raise an AttributeError
| :type row_key: bytes | ||
| :param row_key: The key of the row to read from. | ||
|
|
||
| :type filter_: :class:`.RowFilter` |
There was a problem hiding this comment.
Did we rewrite the RowFilter class?
There was a problem hiding this comment.
The data client originally duplicated the row_filter file, with a few small changes to some of the classes. The shim deleted the original row_filter file, and aliases in the data client classes instead (with a few patches to make sure the API is consistent)
b8af700 to
ce411c0
Compare
**Changes Made:** - Added methods to convert `Row` and `Cell` objects in the data client to `PartialRowData` and `Cell` objects in the legacy client. - Removed legacy client code related to processing `ReadRowResponse` chunks and testing `ReadRowResponse` chunks. - Removed `_update_message_request` from `RowSet` because it's no longer needed to create a `ReadRowQuery` - Rerouted `read_row` and `read_rows` to use their data client counterparts in `table.py`.
ce411c0 to
dad8a76
Compare
3 participants
Migrating over @gkevinzheng PR from bigtable monorepo googleapis/python-bigtable#1299
Original description:
Additional Changes:
Applied some guards against removed semi-private APIs, after internal discussion:
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