Chore: Python-Spanner-Driver | Spannerlib Wrapper | Added Pool class and relevant tests #658
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 establishes the foundational structure for a new Python wrapper around the Go Spanner client shared library. The primary goal is to offer a performant and Pythonic way to interact with Google Cloud Spanner. It introduces core components for managing connection pools, defines the necessary interoperability mechanisms with the Go library using 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
|
![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
The pull request introduces the Pool class and its associated tests, along with the foundational AbstractLibraryObject, SpannerLib (for interacting with the Go shared library), and Message (for C interop) classes. The changes also include necessary build scripts, configuration files (pyproject.toml, noxfile.py), and documentation (README.md).
Overall, the new components are well-structured and include unit and system tests, which is commendable. However, there are several areas for improvement related to maintainability, robustness, and adherence to Python best practices, particularly concerning logging, error handling, type hinting, and resource management.
Key areas for attention include:
- Logging Configuration: Avoid configuring
basicConfigat the library root to prevent interference with user applications. - Error Handling: Ensure consistent and informative error propagation, especially when re-raising exceptions.
- Type Hinting: Address
type: ignorecomments by resolving underlying issues or providing specific justifications. - Resource Management: Refine
__del__implementations andctypesinteractions for better memory safety and clarity. - Build Scripts & Configuration: Review hardcoded values and ensure paths are robust.
Addressing these points will enhance the library's stability, ease of use, and integration into larger Python projects.
...lib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/internal/message.py
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/tests/system/_helper.py
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/tests/system/_helper.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/tests/system/_helper.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/tests/unit/test_pool.py
Show resolved
Hide resolved
| assert "Unexpected error: Something weird happened" in msg | ||
|
|
||
| def test_close_pool_success( | ||
| self, mock_lib_instance, setup_spannerlib_method, mock_msg_data |
There was a problem hiding this comment.
While pylint: disable=redefined-outer-name can be necessary, it's often a sign that variable naming or fixture usage could be improved to avoid shadowing. Consider renaming the mock_msg_data parameter in this test method to avoid the conflict.
|
|
||
| # 3. Assertions | ||
| mock_lib_instance.close_pool.assert_called_once_with(100) | ||
| assert pool._is_disposed is True # pylint: disable=protected-access |
There was a problem hiding this comment.
Directly accessing protected members like _is_disposed is generally discouraged. Consider if there's a more public or intended way to interact with this attribute for testing purposes, or if the test itself could be refactored.
|
|
||
| def test_close_pool_propagates_lib_error( | ||
| self, mock_lib_instance, setup_spannerlib_method, mock_msg_data | ||
| ): # pylint: disable=redefined-outer-name |
There was a problem hiding this comment.
While pylint: disable=redefined-outer-name can be necessary, it's often a sign that variable naming or fixture usage could be improved to avoid shadowing. Consider renaming the mock_msg_data parameter in this test method to avoid the conflict.
| assert "Failed to release" in str(exc_info.value) | ||
|
|
||
| def test_close_pool_wraps_unexpected_error( | ||
| self, mock_lib_instance, setup_spannerlib_method |
There was a problem hiding this comment.
While pylint: disable=redefined-outer-name can be necessary, it's often a sign that variable naming or fixture usage could be improved to avoid shadowing. Consider renaming the mock_msg_data parameter in this test method to avoid the conflict.
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
bhatt4982
force-pushed
the
psd-03-pool
branch
2 times, most recently
from
ad49d51 to
7ad76dc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the spannerlib-python wrapper, including the core Pool class, FFI bindings to the Go shared library, and a comprehensive set of unit and system tests. The overall structure is well-thought-out, with good use of ctypes for interoperability and safety mechanisms like context managers and finalizers to prevent resource leaks.
I've found a critical bug in the error handling that would cause a NameError, a high-severity issue where system tests would fail due to a name mismatch with the documentation, and a couple of medium-severity issues related to dependency and version declarations. My detailed comments provide suggestions to fix these issues.
...rlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/internal/errors.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/tests/system/_helper.py
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/requirements.txt
Show resolved
Hide resolved
bhatt4982
force-pushed
the
psd-03-pool
branch
2 times, most recently
from
3ebfedf to
fc1c516
Compare
…t and system tests, replacing placeholder tests.
…validate production environment variables, copy artifacts in Nox, and clean up docstrings.
…f basic global logging.
bhatt4982
force-pushed
the
psd-03-pool
branch
from
fc1c516 to
4862d24
Compare
bhatt4982
changed the title
olavloite
left a comment
olavloite
left a comment
There was a problem hiding this comment.
LGTM, except for the logging. The library should not log at info level for things that are very normal (such as successfully opening a pool).
(Feel free to address in a follow-up PR)
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/pool.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/pool.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/pool.py
Outdated
Show resolved
Hide resolved
…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.
…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 <[email protected]> * 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 <[email protected]>
* 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.
2 participants
No description provided.