feat: Shimmed sample_row_keys #1252
Conversation
product-auto-label
bot
added
api: bigtable
Summary of ChangesHello @gkevinzheng, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 sample_row_keys method to use the new data client, which is a good step in modernizing the library. However, this change introduces a behavioral difference by switching from a streaming response to buffering all results in memory, which could impact performance for large tables. I've added a comment with more details on this. The accompanying test changes are well-written and correctly validate the new implementation.
| return ( | ||
| data_messages_v2_pb2.SampleRowKeysResponse( | ||
| row_key=row_key, offset_bytes=offset_bytes | ||
| ) | ||
| for row_key, offset_bytes in self._table_impl.sample_row_keys() | ||
| ) |
There was a problem hiding this comment.
This change alters the behavior of sample_row_keys from streaming results to buffering them in memory. The new implementation using self._table_impl.sample_row_keys() collects all row key samples into a list before this method yields them. The previous implementation returned a streaming iterator from the gRPC call.
This can lead to significantly increased memory consumption for tables with a large number of sample keys.
Additionally, the docstring for this method is now inaccurate. It states that the method returns a cancel-able iterator (GrpcRendezvous), but it now returns a generator that is not cancel-able and is backed by an in-memory list. The docstring should be updated to reflect this new behavior and warn about the potential for increased memory usage.
2 participants
Using the new data client for
sample_row_keysFixes #1167