Skip to content

Conversation

@gargsaumya
Copy link
Contributor

@gargsaumya gargsaumya commented May 19, 2025

ADO Task ID AB#37534

This pull request introduces a new header file, ddbc_bindings.h, for managing SQL database interactions in the mssql_python module. The file provides function pointer typedefs for SQL APIs, extern declarations for these function pointers, and utility classes and functions for logging, exception handling, driver loading, and SQL handle management.


Summary

Refactored the native layer to clearly separate binding logic and ODBC utility functions. This decouples the tightly integrated components from ddbc_bindings.cpp. This has been done to improve modularity, reusability, and maintainability of the native C++ layer to support cross-platform development.
We have also introduced core utilities such as SqlHandle and DriverLoader to promote reuse and reduce redundancy.

Checklist

  • Tests Passed (if applicable) : All pytests passed
  • Docs Updated (if necessary) : Not needed

Testing Performed

The changes were verified against all the pytests on Windows. (current support)

Copilot AI review requested due to automatic review settings May 19, 2025 09:07
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 modularizes the native ODBC binding layer by extracting common typedefs, utilities, and wrappers out of ddbc_bindings.cpp into a dedicated header, improving separation of concerns and reuse.

  • Added function pointer typedefs and extern declarations for all ODBC APIs in ddbc_bindings.h.
  • Introduced logging and exception helper declarations (LOG, ThrowStdException, LoadDriverOrThrowException).
  • Added singleton DriverLoader and RAII-style SqlHandle wrapper classes.
Comments suppressed due to low confidence (3)

mssql_python/pybind/ddbc_bindings.h:135

  • [nitpick] Private member naming is inconsistent: DriverLoader uses m_ prefix but here _type uses a leading underscore. Consider using m_type and m_handle to align with m_driverLoaded.
SQLSMALLINT _type;

mssql_python/pybind/ddbc_bindings.h:108

  • [nitpick] The name LOG is very generic and could collide with other logging macros or symbols. Consider a more specific name or namespace qualification (e.g., DDBC_LOG).
void LOG(const std::string& formatString, Args&&... args);

mssql_python/pybind/ddbc_bindings.h:107

  • The template function LOG is only declared here but has no inline definition, which will lead to linker errors. Move its implementation into this header or mark it inline.
template <typename... Args>

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.

The refactoring is a positive step towards better code organization and maintainability. The introduction of the DriverLoader singleton and the move of declarations to a header file are good improvements. The main areas to focus on for further improvement and to ensure robustness are cross-platform compatibility (if applicable), GIL handling for Python callbacks, and thorough testing.

I have left a few comments, let's discuss and resolve them.

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.

Added some context and minor suggestions in places wherever needed.

@gargsaumya gargsaumya force-pushed the saumya/refactor-ddbc branch from d63a44a to 1182190 Compare May 20, 2025 09:07
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.

All looks good. PR ready for merge.

@sumitmsft sumitmsft merged commit c7661d6 into main May 20, 2025
4 checks passed
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