-
Notifications
You must be signed in to change notification settings - Fork 27
Chore: Python-Spanner-Driver - Added Connection class and relevant tests #659
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 groundwork for a high-performance Python wrapper for the Go Spanner client shared library. It establishes the core Python classes for managing database connections and pools, sets up the necessary interoperability with the Go library, and introduces a comprehensive build and testing infrastructure to ensure reliability and maintainability. 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 initial structure for the spannerlib-python wrapper, including the core classes Connection and Pool, FFI bindings, and a comprehensive set of unit and system tests. The overall structure is well-designed, with good use of abstract base classes, protocols for type safety, and context managers for resource management. The FFI layer is carefully implemented with attention to memory safety. The tests are thorough and cover many edge cases. My review includes a few suggestions to improve logging practices, exception handling consistency, and script portability.
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/__init__.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/README.md
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/build-shared-lib.sh
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/samples/requirements.txt
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/tests/system/_helper.py
Outdated
Show resolved
Hide resolved
78174b7 to
3c31855
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 pull request introduces the initial structure for the spannerlib-python wrapper, including the core Connection and Pool classes, FFI bindings, and a comprehensive test suite. The overall design is robust, with excellent attention to resource management and error handling. I've identified one critical issue related to loading the shared library that could cause failures when the package is installed from a wheel, and a few medium-severity suggestions to improve maintainability, consistency, and documentation. Great work on setting up a solid foundation for the project!
.../wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/internal/spannerlib.py
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/README.md
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/README.md
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/build-shared-lib.sh
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/build-shared-lib.sh
Show resolved
Hide resolved
...ppers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/abstract_library_object.py
Outdated
Show resolved
Hide resolved
...erlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/internal/types.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/pool.py
Outdated
Show resolved
Hide resolved
e0780f2 to
67b9299
Compare
… instead of just the pool ID.
…or` and remove `bind_library` calls.
509393a to
952cf95
Compare
…bstractLibraryObject` docstring.
…ction instead of a pool.
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/connection.py
Outdated
Show resolved
Hide resolved
…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.
…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.