-
Notifications
You must be signed in to change notification settings - Fork 35
FEAT: Unify DDBC Bindings - Common Code Consolidation Across Windows/macOS #101
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
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.
Pull Request Overview
This PR centralizes and unifies DDBC bindings for Windows and macOS, enhances logging for debugging, and refactors driver loading and wide-character handling.
- Introduce platform-agnostic driver loading and
GetFunctionPointerhelper - Add detailed debug logs throughout parameter binding and driver initialization
- Implement macOS/Linux wide-character utilities and refactor Windows-specific code
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| mac_utils.h | Add header comment and licensing, introduce utility purpose |
| mac_utils.cpp | Update header and licensing comments |
| ddbc_bindings.h | Add platform checks, DriverHandle typedef, string converters, and remove legacy Windows includes |
| ddbc_bindings.cpp | Refactor driver load with std::filesystem, replace Windows-only code, enhance logging, and unify wide-char handling |
Comments suppressed due to low confidence (3)
mssql_python/pybind/ddbc_bindings.h:25
- The inline functions
SQLWCHARToWStringandWStringToSQLWCHARusestd::vectorbut there is no#include <vector>in this header. Add the<vector>include to avoid compilation errors.
#include <sql.h>
mssql_python/pybind/ddbc_bindings.cpp:785
- In
SQLCheckError_Wrap, only Windows (_WIN32) and macOS (__APPLE__) paths are handled. On Linux (non-Windows, non-Apple), the conversion falls through to the Windows branch. Consider using#if !defined(_WIN32)to cover all non-Windows platforms consistently.
#if defined(_WIN32)
mssql_python/pybind/ddbc_bindings.cpp:193
- [nitpick] These DEBUG-level logs inside tight loops (per-param and per-char) may introduce runtime overhead during bulk operations. Consider reducing verbosity or gating them behind a more specific debug flag.
LOG("Starting parameter binding. Number of parameters: {}", params.size());
|
Thanks for the suggestions @Ramudaykumar. The comments were related to existing functions in removed files, not newly added in this PR. However, the issues have now been addressed - including proper UTF-8 handling, cleaner logging, and return value checks. Let me know if anything else needs changes. |
988b312 to
d9adeae
Compare
Ramudaykumar
left a comment
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.
Looks good to me.
This pull request refines cross-platform compatibility for the
mssql_pythonbindings, improves logging, and simplifies driver loading and function handling. Key changes include replacing Windows-specific code with platform-agnostic alternatives, adding detailed logging for debugging, and introducing macOS-specific handling for wide character conversions.Cross-platform compatibility improvements:
Replaced Windows-specific headers (
<shlwapi.h>) with<filesystem>for path handling and removed directLoadLibraryWcalls in favor of platform-agnostic dynamic library loading (LoadDriverLibrary). [1] [2] [3]Added macOS-specific handling for wide character conversions (
SQLWCHARtowchar_t) in various methods such asSQLExecDirect_wrap,SQLExecute_wrap, andSQLGetData_wrap. This ensures proper functionality on macOS and Linux platforms. [1] [2] [3]Enhanced logging:
Introduced detailed logging throughout the
BindParametersmethod to debug parameter binding, including character code points and buffer sizes. Added logs for macOS-specific SQLWCHAR buffer creation. [1] [2] [3]Added logging for driver loading, including paths and error messages, and improved error reporting with platform-specific implementations of
GetLastErrorMessage. [1] [2]Driver loading simplifications:
<filesystem>for path construction and introduced platform-agnostic functions to load dynamic libraries and retrieve function pointers. Simplified handling of driver function pointers by consolidating repetitive code. [1] [2]Wide character handling improvements:
SQLWCHARToWString,WStringToSQLWCHAR) for macOS/Linux compatibility in methods likeSQLDescribeCol_wrapandSQLGetData_wrap. [1] [2] [3]Code cleanup:
WideToUTF8and legacy Windows-specific driver loading code, reducing clutter and improving maintainability. [1] [2]Issue Reference
AB#37626