FEAT: Unify DDBC Bindings - Common Code Consolidation Across Windows/macOS #101
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
gargsaumya
changed the title
There was a problem hiding this comment.
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. |
gargsaumya
force-pushed
the
saumya/mac-sqlwchar
branch
from
988b312 to
d9adeae
Compare
bewithgaurav
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 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