-
Notifications
You must be signed in to change notification settings - Fork 425
ci(bigtable): update PartialResultSetSource to handle Cord usage #15645
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @mpeddada1, 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 enhances the Bigtable client library's Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the PartialResultSetSource
to handle absl::Cord
usage by introducing an AsString
helper function to convert absl::Cord
to std::string
when necessary. This ensures that the resume_token
is correctly processed, regardless of whether it's stored as a std::string
or an absl::Cord
internally. The changes also include adding necessary header files and adjusting namespace usage.
google/cloud/bigtable/value.h
Outdated
// Some Bigtable proto fields use Cord internally and string externally. | ||
template <typename T, typename std::enable_if< | ||
std::is_same<T, std::string>::value>::type* = nullptr> | ||
std::string AsString(T const& s); | ||
|
||
template <typename T, typename std::enable_if< | ||
std::is_same<T, std::string>::value>::type* = nullptr> | ||
std::string AsString(T&& s); | ||
|
||
template <typename T, typename std::enable_if< | ||
std::is_same<T, absl::Cord>::value>::type* = nullptr> | ||
std::string AsString(T const& s); | ||
|
||
template <typename T, typename std::enable_if< | ||
std::is_same<T, absl::Cord>::value>::type* = nullptr> | ||
std::string AsString(T&& s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These function declarations are missing the inline
keyword. According to the Google C++ Style Guide1, small, frequently used functions should be declared inline to potentially improve performance.
inline std::string AsString(T const& s);
inline template <typename T, typename std::enable_if<std::is_same<T, std::string>::value>::type*>
inline std::string AsString(T&& s);
inline template <typename T, typename std::enable_if<std::is_same<T, absl::Cord>::value>::type*>
inline std::string AsString(T const& s);
inline template <typename T, typename std::enable_if<std::is_same<T, absl::Cord>::value>::type*>
inline std::string AsString(T&& s);
Style Guide References
Footnotes
-
This repository mostly follows the Google C++ style guide found at https://google.github.io/styleguide/cppguide.html. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #15645 +/- ##
==========================================
- Coverage 93.11% 93.10% -0.01%
==========================================
Files 2433 2433
Lines 223720 223810 +90
==========================================
+ Hits 208312 208387 +75
- Misses 15408 15423 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.