Skip to content

Conversation

@gargsaumya
Copy link
Contributor

@gargsaumya gargsaumya commented May 29, 2025

This pull request introduces connection pooling functionality to the mssql_python library. The changes include adding a new module to manage pooling configuration, modifying existing connection logic to support pooling, and extending the C++ bindings to handle the new pooling parameter.

Connection pooling feature:

  • mssql_python/pool_config.py: Added a new module to manage connection pooling, including a global _pooling_enabled flag and an enable_pooling function to configure pooling parameters.

Integration of pooling into Python connection logic:

  • mssql_python/connection.py: Updated the Connection class to include the _pooling_enabled flag when creating a connection, ensuring pooling is respected in subsequent operations.
  • mssql_python/__init__.py: Imported the enable_pooling function from the new pool_config module to make it accessible at the package level.

Extension of C++ bindings for pooling:

  • mssql_python/pybind/connection/connection.cpp and connection.h: Modified the Connection constructor to accept a new use_pooling parameter and store it as _usePool. [1] [2]
  • mssql_python/pybind/ddbc_bindings.cpp: Updated the Pybind11 bindings for the Connection class to include the use_pooling parameter, allowing Python code to pass pooling configuration to the underlying C++ implementation.

Checklist

  • Tests Passed (if applicable) : All pytests are passing

Copilot AI review requested due to automatic review settings May 29, 2025 13:31
@gargsaumya gargsaumya changed the base branch from main to saumya/integratec++class May 29, 2025 13:32
@gargsaumya gargsaumya force-pushed the saumya/pool-python branch from 2be7638 to c74ad07 Compare May 29, 2025 13:34
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 refactors the mssql_python driver to use a unified C++ ddbc_bindings.Connection class for all connection operations, removes legacy handle‐management code, and adds a placeholder API for connection pooling.

  • Swap out manual ODBC handle setup/teardown for ddbc_bindings.Connection
  • Introduce pool_config.enable_pooling and pass pooling flag into new C++ Connection
  • Update cursor to call into Connection.alloc_statement_handle and clean up redundant logic

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/test_005_exceptions.py Updated expected exception type/message for invalid connection
mssql_python/pybind/ddbc_bindings.h Added includes, mutex, ErrorInfo struct, and once_flag
mssql_python/pybind/ddbc_bindings.cpp Removed old wrap functions, adjusted SqlHandle destructor, bound new Connection API
mssql_python/pybind/connection/connection.h Introduced new C++ Connection class signature and members
mssql_python/pybind/connection/connection.cpp Fully implemented Connection logic, error handling, and pooling flag
mssql_python/pool_config.py New module with enable_pooling stub
mssql_python/cursor.py Use Connection.alloc_statement_handle() and tidy free logic
mssql_python/connection.py Swap to using C++ Connection, remove legacy handle methods
mssql_python/init.py Exported enable_pooling
Comments suppressed due to low confidence (1)

mssql_python/pybind/ddbc_bindings.h:15

  • [nitpick] m_driverLoaded is never initialized or used after switching to std::call_once. Remove it or initialize/use it consistently to avoid confusion.
bool m_driverLoaded;

@gargsaumya gargsaumya requested a review from sumitmsft May 29, 2025 13:37
@gargsaumya gargsaumya changed the title Saumya/pool python FEAT: Python support for Pooling May 29, 2025
@microsoft microsoft deleted a comment from Copilot AI May 29, 2025
@microsoft microsoft deleted a comment from Copilot AI May 29, 2025
@gargsaumya gargsaumya changed the base branch from saumya/integratec++class to main May 29, 2025 15:33
Copy link
Contributor

@sumitmsft sumitmsft left a comment

Choose a reason for hiding this comment

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

I have added a few comments to keep a note of. Please see if you can incorporate them when you make changes to ddbc_bindings.cpp.

@gargsaumya gargsaumya force-pushed the saumya/pool-python branch 3 times, most recently from d1f8009 to 856915a Compare May 30, 2025 06:54
@gargsaumya gargsaumya force-pushed the saumya/pool-python branch from 856915a to daa63ed Compare May 30, 2025 06:56
@gargsaumya gargsaumya requested a review from sumitmsft May 30, 2025 06:57
Copy link
Collaborator

@bewithgaurav bewithgaurav left a comment

Choose a reason for hiding this comment

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

Approved with some minor suggestions for refactoring, can be added as a TODO for later use.

@sumitmsft sumitmsft merged commit 047953c into main May 30, 2025
4 checks passed
@gargsaumya gargsaumya mentioned this pull request Jun 2, 2025
7 tasks
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.

4 participants