Skip to content

Conversation

@gargsaumya
Copy link
Contributor

@gargsaumya gargsaumya commented May 19, 2025

ADO Task ID [AB#34962]
This pull request introduces a new Connection class for managing MSSQL database connections in the mssql_python module. The class provides methods for connecting, disconnecting, handling transactions, and managing autocommit settings. Below are the key changes:

Implementation of the Connection class:

  • Header file (connection.h):

    • Added the Connection class definition, including methods for connecting (connect), disconnecting (close), transaction management (commit, rollback, end_transaction), and autocommit settings (set_autocommit, get_autocommit).
    • Introduced member variables such as _conn_str for the connection string, _env_handle and _dbc_handle for SQL handles, and _autocommit to track autocommit mode.
  • Source file (connection.cpp):

    • Added the Connection class constructor, destructor, and method stubs with logging statements for key operations like connecting, disconnecting, committing, rolling back, and managing autocommit.

Checklist

  • Tests Passed (if applicable) : All pytests passed successfully.
  • Docs Updated (if necessary) Will be added in upcoming PRs once the Connection management is fully integrated in c++.

Copilot AI review requested due to automatic review settings May 19, 2025 10:56
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 provides a foundational implementation for a C++ MSSQL connection layer by introducing a new Connection class together with related utility functions and function pointer typedefs for ODBC API interactions.

  • Added ddbc_bindings.h with various function pointer typedefs and external declarations for ODBC APIs.
  • Introduced Connection class header and a minimal implementation in connection.cpp with placeholder methods for connection management.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
mssql_python/pybind/ddbc_bindings.h Introduces ODBC API function pointer typedefs and helper functions.
mssql_python/pybind/connection/connection.h Defines the Connection class interface for managing MSSQL connections.
mssql_python/pybind/connection/connection.cpp Provides placeholder implementations for Connection methods.
Comments suppressed due to low confidence (1)

mssql_python/pybind/connection/connection.h:29

  • [nitpick] The member variable '_conn' in the Connection class is ambiguous, as its purpose is unclear given the context. Consider reviewing its necessity or renaming it to better reflect its intended use.
std::shared_ptr<Connection> _conn;

@gargsaumya gargsaumya force-pushed the saumya/connectionclassc++ branch from 6313410 to be11908 Compare May 19, 2025 11:10
@gargsaumya gargsaumya changed the title Saumya/connectionclassc++ FEAT: add Connection class in C++ May 19, 2025
@gargsaumya gargsaumya changed the title FEAT: add Connection class in C++ FEAT: add Connection class structure in C++ May 19, 2025
@gargsaumya gargsaumya changed the base branch from main to saumya/refactor-ddbc May 20, 2025 03:45
@gargsaumya gargsaumya changed the title FEAT: add Connection class structure in C++ FEAT: adding connection class structure in C++ May 20, 2025
@gargsaumya gargsaumya force-pushed the saumya/connectionclassc++ branch from 7727ace to eb7161e Compare May 20, 2025 10:29
@gargsaumya gargsaumya requested review from Copilot and sumitmsft May 20, 2025 11:05
@gargsaumya gargsaumya changed the base branch from saumya/refactor-ddbc to main May 20, 2025 11:06
@gargsaumya gargsaumya changed the base branch from main to saumya/refactor-ddbc May 20, 2025 11: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 adds a new C++ Connection class to manage MSSQL database connectivity in the mssql_python module, including stubs for connection, transaction, and autocommit operations.

  • Introduces Connection class definition in connection.h with handles, connection string, and autocommit state.
  • Adds method stubs and lifecycle logging in connection.cpp.
  • Sets up the basic structure for future implementation of ODBC operations.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
mssql_python/pybind/connection/connection.h Defines Connection class, members, and method signatures
mssql_python/pybind/connection/connection.cpp Implements constructor, destructor, and stubs for methods with logging
Comments suppressed due to low confidence (2)

mssql_python/pybind/connection/connection.h:7

  • [nitpick] Use a more specific include guard macro (e.g. MSSQL_PYTHON_PYBIND_CONNECTION_CONNECTION_H) to avoid collisions.
#ifndef CONNECTION_H

mssql_python/pybind/connection/connection.h:10

  • Missing headers for std::wstring and std::shared_ptr. Add #include <string> and #include <memory>.
#include "ddbc_bindings.h"

@microsoft microsoft deleted a comment from Copilot AI May 20, 2025
@microsoft microsoft deleted a comment from Copilot AI May 20, 2025
@microsoft microsoft deleted a comment from Copilot AI May 20, 2025
@gargsaumya gargsaumya changed the base branch from saumya/refactor-ddbc to main May 20, 2025 12:14
@gargsaumya gargsaumya force-pushed the saumya/connectionclassc++ branch from daf84f8 to eca18a0 Compare May 20, 2025 12:24
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.

This PR is just structure of connection class, more comprehensive review will be required to be done once we have the implementation.

@gargsaumya gargsaumya requested a review from bewithgaurav May 21, 2025 05:29
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.

As discussed, we have finalized on the below approach:

  • Connection is encapsulated in two classes:
  • ddbc_bindings will have a _Connection class and Python will be using that class and wrap it into its own connection class
  • this is being done since we want to implement connection pooling in cpp rn, eventually we can think of doing a complete migration, and eliminate python layer altogether maybe
  • also, this is being done keeping in mind Cursor will also follow the same changes
    Approving this under the above context

@gargsaumya gargsaumya merged commit 292d8e9 into main May 21, 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