-
Notifications
You must be signed in to change notification settings - Fork 27
Chore: Python-Spanner-Driver - Added Rows class, its methods and relevant tests... #661
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 @bhatt4982, 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 lays the foundational groundwork for a new Python client library, 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 introduces the core Python wrapper for the Spanner Go shared library, including the Pool, Connection, and Rows classes, along with their ctypes-based internal implementation and a comprehensive set of unit and system tests. The overall structure is well-designed, particularly the use of an abstract base class for resource management and the safety nets for handling unclosed resources. My review focuses on improving error handling consistency, clarifying documentation and log messages, and making the code more robust by avoiding silent failures and brittle exception checks. These changes will enhance the library's maintainability and developer experience.
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/rows.py
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/rows.py
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/connection.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/connection.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/connection.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/rows.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/rows.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/tests/system/_setup_env.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/tests/system/_setup_env.py
Outdated
Show resolved
Hide resolved
8c20d7a to
ab05f61
Compare
… including emulator setup.
…ount()` methods to the `Rows` class for fetching data and statistics, including unit tests.
…ate count, and row iteration.
…o reflect protobuf data handling.
97ebff4 to
6c776f0
Compare
|
/gemini review |
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 is a comprehensive pull request that introduces a new Python wrapper for the Spanner Go library. The code is well-structured, with a clear separation of concerns between the FFI layer, public-facing objects, and utility components. The use of abstract base classes for resource management, context managers, and finalizers (__del__) to warn about unclosed resources demonstrates a strong focus on correctness and memory safety, which is crucial for a C-extension wrapper. The unit and system tests are thorough and provide good coverage. I've identified a few areas for improvement, mainly concerning a potential build script issue, consistency in error handling, and a logic bug in update_count. Overall, this is an excellent foundation for the new library.
spannerlib/wrappers/spannerlib-python/spannerlib-python/build-shared-lib.sh
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/rows.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/connection.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/connection.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/connection.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/rows.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/rows.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/rows.py
Outdated
Show resolved
Hide resolved
…via `_pb` and improve system test DML uniqueness.
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/rows.py
Outdated
Show resolved
Hide resolved
…ion class and relevant tests... (#662) * feat: Add transaction management with `begin_transaction`, `commit`, and `rollback` methods and their respective tests. * feat: add system and unit tests for transaction mutation writes, including commit and rollback scenarios.
… unavailable and updates the corresponding test.
…ecuteBatch and WriteMutation methods and relevant tests... (#660) * feat: Introduce `Rows` class and add `execute`, `execute_batch`, and `write_mutations` methods to `Connection`. * feat: Add SQL execution, batch DML, and mutation writing methods to Connection and expose Rows object. * Chore: Python-Spanner-Driver - Added Rows class, its methods and relevant tests... (#661) * test: add system tests for query, batch DML, and mutation operations, including emulator setup. * feat: Add `next()`, `metadata()`, `result_set_stats()`, and `update_count()` methods to the `Rows` class for fetching data and statistics, including unit tests. * test: Add system tests for `Rows` class covering metadata, stats, update count, and row iteration. * Chore: Python-Spanner-Driver - Added transaction functions in Connection class and relevant tests... (#662) * feat: Add transaction management with `begin_transaction`, `commit`, and `rollback` methods and their respective tests. * feat: add system and unit tests for transaction mutation writes, including commit and rollback scenarios. * feat: `update_count` returns -1 instead of 0 when row count stats are unavailable and updates the corresponding test. * refactor: remove direct Pool argument from Rows constructor and access via Connection. * feat: Add `IF NOT EXISTS` to `CREATE TABLE` and `IF EXISTS` to `DROP TABLE` statements, removing explicit exception handling. * feat: `write_mutations` now returns `None` for buffered mutations, updating its return type and related tests.
…sts (#659) * feat: introduce Connection class and enable Pool to create and manage connections * Chore: Python-Spanner-Driver | Spannerlib Wrapper | Added Execute, ExecuteBatch and WriteMutation methods and relevant tests... (#660) * feat: Introduce `Rows` class and add `execute`, `execute_batch`, and `write_mutations` methods to `Connection`. * feat: Add SQL execution, batch DML, and mutation writing methods to Connection and expose Rows object. * Chore: Python-Spanner-Driver - Added Rows class, its methods and relevant tests... (#661) * test: add system tests for query, batch DML, and mutation operations, including emulator setup. * feat: Add `next()`, `metadata()`, `result_set_stats()`, and `update_count()` methods to the `Rows` class for fetching data and statistics, including unit tests. * test: Add system tests for `Rows` class covering metadata, stats, update count, and row iteration. * Chore: Python-Spanner-Driver - Added transaction functions in Connection class and relevant tests... (#662) * feat: Add transaction management with `begin_transaction`, `commit`, and `rollback` methods and their respective tests. * feat: add system and unit tests for transaction mutation writes, including commit and rollback scenarios. * feat: `update_count` returns -1 instead of 0 when row count stats are unavailable and updates the corresponding test. * refactor: remove direct Pool argument from Rows constructor and access via Connection. * feat: Add `IF NOT EXISTS` to `CREATE TABLE` and `IF EXISTS` to `DROP TABLE` statements, removing explicit exception handling. * feat: `write_mutations` now returns `None` for buffered mutations, updating its return type and related tests.
…and relevant tests (#658) * feat: Introduce AbstractLibraryObject and Pool modules with their unit and system tests, replacing placeholder tests. * Chore: Python-Spanner-Driver - Added Connection class and relevant tests (#659) * feat: introduce Connection class and enable Pool to create and manage connections * Chore: Python-Spanner-Driver | Spannerlib Wrapper | Added Execute, ExecuteBatch and WriteMutation methods and relevant tests... (#660) * feat: Introduce `Rows` class and add `execute`, `execute_batch`, and `write_mutations` methods to `Connection`. * feat: Add SQL execution, batch DML, and mutation writing methods to Connection and expose Rows object. * Chore: Python-Spanner-Driver - Added Rows class, its methods and relevant tests... (#661) * test: add system tests for query, batch DML, and mutation operations, including emulator setup. * feat: Add `next()`, `metadata()`, `result_set_stats()`, and `update_count()` methods to the `Rows` class for fetching data and statistics, including unit tests. * test: Add system tests for `Rows` class covering metadata, stats, update count, and row iteration. * Chore: Python-Spanner-Driver - Added transaction functions in Connection class and relevant tests... (#662) * feat: Add transaction management with `begin_transaction`, `commit`, and `rollback` methods and their respective tests. * feat: add system and unit tests for transaction mutation writes, including commit and rollback scenarios. * feat: `update_count` returns -1 instead of 0 when row count stats are unavailable and updates the corresponding test. * refactor: remove direct Pool argument from Rows constructor and access via Connection. * feat: Add `IF NOT EXISTS` to `CREATE TABLE` and `IF EXISTS` to `DROP TABLE` statements, removing explicit exception handling. * feat: `write_mutations` now returns `None` for buffered mutations, updating its return type and related tests.
…dle spannerlib library interaction (#657) * feat: Add initial Python wrapper for Spanner library, including internal message structures, error handling, and a protocol definition. * refactor: Introduce `_load_library` for library initialization, remove `_get_lib_path`, and update related tests. * Chore: Python-Spanner-Driver | Spannerlib Wrapper | Added Pool class and relevant tests (#658) * feat: Introduce AbstractLibraryObject and Pool modules with their unit and system tests, replacing placeholder tests. * Chore: Python-Spanner-Driver - Added Connection class and relevant tests (#659) * feat: introduce Connection class and enable Pool to create and manage connections * Chore: Python-Spanner-Driver | Spannerlib Wrapper | Added Execute, ExecuteBatch and WriteMutation methods and relevant tests... (#660) * feat: Introduce `Rows` class and add `execute`, `execute_batch`, and `write_mutations` methods to `Connection`. * feat: Add SQL execution, batch DML, and mutation writing methods to Connection and expose Rows object. * Chore: Python-Spanner-Driver - Added Rows class, its methods and relevant tests... (#661) * test: add system tests for query, batch DML, and mutation operations, including emulator setup. * feat: Add `next()`, `metadata()`, `result_set_stats()`, and `update_count()` methods to the `Rows` class for fetching data and statistics, including unit tests. * test: Add system tests for `Rows` class covering metadata, stats, update count, and row iteration. * Chore: Python-Spanner-Driver - Added transaction functions in Connection class and relevant tests... (#662) * feat: Add transaction management with `begin_transaction`, `commit`, and `rollback` methods and their respective tests. * feat: add system and unit tests for transaction mutation writes, including commit and rollback scenarios. * feat: `update_count` returns -1 instead of 0 when row count stats are unavailable and updates the corresponding test. * refactor: remove direct Pool argument from Rows constructor and access via Connection. * feat: Add `IF NOT EXISTS` to `CREATE TABLE` and `IF EXISTS` to `DROP TABLE` statements, removing explicit exception handling. * feat: `write_mutations` now returns `None` for buffered mutations, updating its return type and related tests. * Update spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/internal/message.py Co-authored-by: Knut Olav Løite <koloite@gmail.com> * feat: Remove Python 3.8 fallback for `importlib.resources` and few docstring update * feat: Enhance SpannerLibError messages to include gRPC status names and add corresponding tests. * test: update SpannerLibError repr test message and code values --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Knut Olav Løite <koloite@gmail.com>
* chore: Create setup for SpannerLib-Python project * Chore: Python-Spanner-Driver | Spannerlib Wrapper | Added code to handle spannerlib library interaction (#657) * feat: Add initial Python wrapper for Spanner library, including internal message structures, error handling, and a protocol definition. * Chore: Python-Spanner-Driver | Spannerlib Wrapper | Added Pool class and relevant tests (#658) * feat: Introduce AbstractLibraryObject and Pool modules with their unit and system tests, replacing placeholder tests. * Chore: Python-Spanner-Driver - Added Connection class and relevant tests (#659) * feat: introduce Connection class and enable Pool to create and manage connections * Chore: Python-Spanner-Driver | Spannerlib Wrapper | Added Execute, ExecuteBatch and WriteMutation methods and relevant tests... (#660) * feat: Introduce `Rows` class and add `execute`, `execute_batch`, and `write_mutations` methods to `Connection`. * feat: Add SQL execution, batch DML, and mutation writing methods to Connection and expose Rows object. * Chore: Python-Spanner-Driver - Added Rows class, its methods and relevant tests... (#661) * test: add system tests for query, batch DML, and mutation operations, including emulator setup. * feat: Add `next()`, `metadata()`, `result_set_stats()`, and `update_count()` methods to the `Rows` class for fetching data and statistics, including unit tests. * test: Add system tests for `Rows` class covering metadata, stats, update count, and row iteration. * Chore: Python-Spanner-Driver - Added transaction functions in Connection class and relevant tests... (#662) * feat: Add transaction management with `begin_transaction`, `commit`, and `rollback` methods and their respective tests. * feat: add system and unit tests for transaction mutation writes, including commit and rollback scenarios.
No description provided.