FEAT: complete macOS-support with main ddbc_bindings and connection pooling #102
+23
−1,095
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR unifies and refactors ODBC connection handling for both macOS and Windows by removing the old macOS-only implementation, consolidating platform-specific logic into shared modules, and enhancing debugging through detailed logging of parameter binding.
- Consolidated connection logic: removed
connection_mac.pyand related CMake logic, updatedConnectionHandleto use UTF-8 strings everywhere - Added wide-char/UTF-8 conversion utilities and detailed parameter-binding logs in
ddbc_bindings.* - Swapped Windows‐only headers for
std::filesystemand introduced platform‐agnostic driver‐loading helpers
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pybind/mac_utils.h | Added license, header comments for macOS wide-char utilities |
| pybind/ddbc_bindings.h | Unified platform includes and added inline UTF-8/wstring helpers |
| pybind/ddbc_bindings.cpp | Swapped to std::filesystem, added detailed logging, driver helpers |
| pybind/connection/connection.* | Updated ConnectionHandle to accept std::string and convert to wide |
| pybind/CMakeLists.txt | Removed macOS-specific blocks, always build unified source |
| db_connection.py | Dropped conditional import of macOS connection implementation |
| connection_pool.cpp | Removed unused <iostream> include |
Comments suppressed due to low confidence (2)
mssql_python/pybind/ddbc_bindings.h:49
- The header uses std::vector but does not include . Add "#include " near the top to ensure this compiles.
inline std::vector<SQLWCHAR> WStringToSQLWCHAR(const std::wstring& str) {
mssql_python/pybind/ddbc_bindings.h:20
- [nitpick] The IS_WINDOWS macro is defined but never referenced elsewhere; consider removing it or using it consistently to reduce unused code.
#define IS_WINDOWS 1
gargsaumya
changed the title
gargsaumya
changed the title
gargsaumya
changed the title
|
Ensure that every SQL call (e.g., SQLAllocHandle_ptr, SQLDriverConnect_ptr) is followed by proper error handling using checkError(ret); or equivalent logic. |
|
I can't see test.py for DDBC covering connect_pool changes, consider adding those. |
Yes, those have been added in another PR. #103 |
Ramudaykumar
previously approved these changes
Jun 27, 2025
bewithgaurav
previously approved these changes
Jun 27, 2025
gargsaumya
dismissed stale reviews from bewithgaurav and Ramudaykumar
The base branch was changed.
gargsaumya
force-pushed
the
saumya/mac-full-support
branch
from
f6f1af8 to
1dbd1fb
Compare
gargsaumya
force-pushed
the
saumya/mac-full-support
branch
3 times, most recently
from
c35629f to
231c4b0
Compare
gargsaumya
force-pushed
the
saumya/mac-full-support
branch
from
231c4b0 to
a41f35d
Compare
bewithgaurav
approved these changes
Jun 27, 2025
jahnvi480
approved these changes
Jun 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request refactors the
mssql_pythonmodule to remove macOS-specific implementations and unify the codebase for better maintainability. Key changes include removing theconnection_mac.pyfile, updating connection handling to use UTF-8 strings, and modifying build configurations to eliminate platform-specific blocks.Removal of macOS-specific implementations:
mssql_python/connection_mac.py: Deleted the entire file, which contained macOS-specific implementations of theConnectionclass. This eliminates platform-specific handling in favor of a unified approach.mssql_python/db_connection.py: Removed conditional imports for macOS and updated theConnectionimport to use the unified implementation.mssql_python/pybind/CMakeLists.txt: Removed the conditional block for macOS-specific source files (ddbc_bindings_mac.cpp) and standardized the build process to useddbc_bindings.cpp.Transition to UTF-8 string handling:
mssql_python/pybind/connection/connection.cpp: UpdatedConnectionHandleto convert connection strings from UTF-8 to wide strings (std::wstring) internally, ensuring compatibility across platforms.mssql_python/pybind/connection/connection.h: Changed the constructor ofConnectionHandleto accept UTF-8 strings (std::string) instead of wide strings (std::wstring).mssql_python/pybind/ddbc_bindings.cpp: Updated the Python bindings forConnectionHandleto accept UTF-8 strings for the connection string parameter.Minor adjustments:
mssql_python/pybind/connection/connection_pool.cpp: Removed an unused#include <iostream>statement to clean up the code.Issue Reference
AB#37666
AB#37610