refactor(bigtable): reorganize query public types and files#15744
refactor(bigtable): reorganize query public types and files#15744scotthart merged 3 commits intogoogleapis:mainfrom
Conversation
Summary of ChangesHello @scotthart, 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 undertakes a significant refactoring effort to improve the organization and clarity of query-related types within the Bigtable and Spanner client libraries. By renaming a generic "results" header to a more specific "result source interface" and consolidating all stream-related classes and iterators into new "row stream" files, the changes aim to create a more intuitive and maintainable codebase. This reorganization helps delineate responsibilities and makes it easier to locate and understand the components involved in processing query results. 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.
Code Review
This pull request refactors the Bigtable client library by reorganizing query-related public types. results.h is renamed to result_source_interface.h, and various stream and iterator classes are consolidated into new row_stream.h and row_stream.cc files. The changes are well-structured and improve the organization of the code.
My review includes a few points:
- A critical fix for missing header includes in the new
row_stream.hfile, which would cause compilation errors. - A suggestion to remove unused headers in
row_stream.cc. - A high-severity finding regarding a likely accidental and inconsistent file rename in the
spannercomponent, which is out of scope for this PR.
Overall, this is a good refactoring. Addressing these points will ensure the correctness and consistency of the codebase.
| #include "google/cloud/bigtable/result_source_interface.h" | ||
| #include "google/cloud/bigtable/version.h" | ||
| #include "google/cloud/status_or.h" | ||
| #include <memory> |
There was a problem hiding this comment.
This header is missing some includes that were present in query_row.h and are still needed for the moved code to compile:
google/cloud/internal/make_status.h: forGetSingularRow.<utility>: forstd::moveused inGetSingularRowandTupleStreamIterator.<functional>: forstd::functionused inRowStreamIterator::Source.<iterator>: forstd::input_iterator_tagused inRowStreamIteratorandTupleStreamIterator.
#include <memory>
#include "google/cloud/internal/make_status.h"
#include <functional>
#include <iterator>
#include <utility>
google/cloud/bigtable/row_stream.cc
Outdated
| #include "google/cloud/internal/make_status.h" | ||
| #include "google/cloud/log.h" | ||
| #include "google/cloud/status.h" | ||
| #include "google/cloud/status_or.h" | ||
| #include <algorithm> |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15744 +/- ##
=======================================
Coverage 93.03% 93.04%
=======================================
Files 2445 2448 +3
Lines 226036 226036
=======================================
+ Hits 210301 210307 +6
+ Misses 15735 15729 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
eafeb92
renames results.h to result_source_interface.h
collects all the RowStream, TupleStream, and associated iterators into a single header file row_stream.h
moves tests around appropriately