Skip to content

Conversation

@gargsaumya
Copy link
Contributor

This pull request introduces significant changes to the mssql_python library, including the addition of connection pooling functionality, refactoring of the connection and cursor handling, and simplifications to the codebase. The most important changes are grouped into themes below.

Connection Pooling:

  • Added connection pooling support by introducing the enable_pooling function and modifying the Connection class to accept a use_pool parameter. Pooling configuration is managed globally and integrated into the connection initialization process. (main.py [1] mssql_python/__init__.py [2] mssql_python/db_connection.py [3] [4] mssql_python/mssql_python.pyi [5]

Refactoring Connection Class:

  • Refactored the Connection class to replace manual resource allocation and attribute handling with direct calls to ddbc_bindings.Connection. Removed redundant methods such as _initializer, _allocate_environment_handle, _allocate_connection_handle, and _connect_to_db. (mssql_python/connection.py [1] [2] [3]
  • Simplified transaction management by delegating commit and rollback operations directly to the ddbc_bindings.Connection object. (mssql_python/connection.py [1] [2]
  • Updated the autocommit property and setautocommit method to use the ddbc_bindings.Connection API for managing autocommit mode. (mssql_python/connection.py [1] [2]

Refactoring Cursor Class:

  • Modified the Cursor class to allocate statement handles directly via the ddbc_bindings.Connection object, removing dependency on the hdbc handle from the Connection class. (mssql_python/cursor.py [1] [2]
  • Removed redundant logging and exception handling for closed connections in cursor operations. (mssql_python/cursor.py mssql_python/cursor.pyL560-R553)

Codebase Simplification:

  • Removed unused attributes and methods related to connection handle management, including _is_closed, _apply_attrs_before, and _set_connection_attributes. (mssql_python/connection.py mssql_python/connection.pyL104-R113)
  • Updated the CMakeLists.txt file to include the new connection_pool.cpp file for connection pooling implementation in the ddbc_bindings module. (mssql_python/pybind/CMakeLists.txt mssql_python/pybind/CMakeLists.txtL93-R93)

Checklist

  • Tests Passed (if applicable)
  • Code is formatted
  • Docs Updated (if necessary)

Testing Performed

  • Unit Tests
  • Manual Testing
    • Python Version: <mention Python version>
    • OS: <mention OS>

Additional Notes

Copilot AI review requested due to automatic review settings May 29, 2025 05:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces connection pooling support and refactors the Python Connection/Cursor classes to delegate to the new C++ binding ConnectionHandle, while removing redundant handle-allocation code.

  • Adds a global enable_pooling API and C++ ConnectionPool/ConnectionPoolManager
  • Refactors Connection/Cursor to use ddbc_bindings.ConnectionHandle and simplifies transaction/handle logic
  • Removes legacy allocation wrappers and disables outdated connection tests

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/test_005_exceptions.py Disabled legacy connection error test (commented out)
tests/test_003_connection.py Disabled invalid-connection and close tests (commented out)
mssql_python/pybind/ddbc_bindings.h Updated pybind11 includes & added default constructor for SqlHandle
mssql_python/pybind/ddbc_bindings.cpp Streamlined SqlHandle destructor, removed redundant wrapper fns & exposed ConnectionHandle
mssql_python/pybind/connection/connection_pool.h Added ConnectionPool and ConnectionPoolManager interfaces
mssql_python/pybind/connection/connection_pool.cpp Implemented pool acquire/release logic with commented debug blocks
mssql_python/pybind/connection/connection.h Refactored C++ Connection/ConnectionHandle interfaces for pooling
mssql_python/pybind/CMakeLists.txt Included connection_pool.cpp in the ddbc_bindings build
mssql_python/mssql_python.pyi Added stub for enable_pooling
mssql_python/db_connection.py Introduced module‐level _pooling_enabled and passed use_pool flag
mssql_python/cursor.py Updated _allocate_statement_handle and _reset_cursor to use new API
mssql_python/connection.py Delegated Python Connection to C++ binding and added use_pool
mssql_python/init.py Exported enable_pooling at package level
main.py Added example usage of pooling and demonstration script
Comments suppressed due to low confidence (3)

tests/test_005_exceptions.py:127

  • A critical error handling test was commented out, reducing coverage for connection errors; consider re-enabling or replacing it with a pooling-aware test.
# def test_connection_error(db_connection):

tests/test_003_connection.py:171

  • Key connection validation tests were commented out, lowering test coverage; consider restoring tests or adding new cases for pooled connections.
# def test_invalid_connection_string():

mssql_python/pybind/connection/connection_pool.cpp:7

  • [nitpick] There are extensive commented-out debug blocks throughout this file; consider removing dead code to improve readability and maintainability.
// std::wcout << L"[POOL] Created new pool. ConnStr: " << _conn_str 

# pooling.py
_pooling_enabled = False

def enable_pooling():
Copy link

Copilot AI May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enable_pooling function signature doesn't accept max_size or idle_timeout parameters as advertised in the docs and pyi, leading to an API mismatch.

Copilot uses AI. Check for mistakes.
"""
...

def enable_pooling(max_size: int, idle_timeout: int) -> None:
Copy link

Copilot AI May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stub signature for enable_pooling expects two parameters, but the implementation takes none; sync the pyi with the actual function signature or update the implementation.

Copilot uses AI. Check for mistakes.
rows = cursor1.fetchone()
print (rows)

print(conn1._conn)
Copy link

Copilot AI May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The sample script accesses a private attribute _conn; consider exposing a public API or removing debug prints to avoid relying on internal implementation details.

Suggested change
print(conn1._conn)
print(conn1.get_connection())

Copilot uses AI. Check for mistakes.
@gargsaumya gargsaumya changed the title Saumya/pool pooling support Jun 2, 2025
@gargsaumya
Copy link
Contributor Author

No need for this, merged following PRs:
#62
#64

@gargsaumya gargsaumya closed this Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants